fix(appsec): send the normalized status code on end-invocation - #817
Open
CarlesDD wants to merge 1 commit into
Open
fix(appsec): send the normalized status code on end-invocation#817CarlesDD wants to merge 1 commit into
CarlesDD wants to merge 1 commit into
Conversation
| endInvocationChannel.publish({ | ||
| span, | ||
| statusCode: result?.statusCode?.toString(), | ||
| statusCode: statusCode ?? result?.statusCode?.toString(), |
Contributor
There was a problem hiding this comment.
is it possible that statusCode has different value from results.statusCode, assuming neither is null?
litianningdatadog
left a comment
litianningdatadog
left a comment
Contributor
There was a problem hiding this comment.
left some comment
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Reorders
onEndingInvocationso the HTTP status code is resolved and tagged before AppSec runs, and passes that normalized value toprocessAppsecResponseinstead of letting it readresult.statusCoderaw.Motivation
extractHTTPStatusCodeTagnormalizes cases the raw result does not cover: ALB and API Gateway v2 handlers may omit the status (defaults to200), a buffered function returning nothing is a502, and a streaming one is a200. AppSec was publishing the raw value, so the WAF received a wrong or missing status for those triggers.The 5xx early return stays nested on
inferredSpan, so its behaviour is unchanged; it just moved behind the AppSec call so 5xx responses still reach the WAF.APPSEC-68818
Testing Guidelines
src/appsec/index.spec.tscovers the normalized value winning over the raw one and the fallback for non-HTTP triggers.src/trace/listener.spec.tsadds cases over real event samples: API Gateway v2 without a status, ALB with no result, streaming with no result, the tag-before-AppSec ordering, and a 500 still reaching AppSec while short-circuiting.src/appsec+src/trace: 475 tests passing.tsc,tslintandprettierclean.Additional Notes
Shippable on its own: it fixes the status code the WAF receives today, with or without API Security. It is also a prerequisite for API Security on Lambda, where the sampling decision is taken off the span and needs
http.status_codealready set.Types of Changes
Check all that apply