Repository navigation
fix(security): clear open high-severity CodeQL alerts (SEC-X1) - #1534
Merged
Merged
Conversation
…VTL int narrowing, VCR headers) Drop the unreleased bare SHA-256 password fallback in Cognito, bounds-check VTL int conversions to the int32 range, and pin VCR record-mode headers at the body write.
NitinKumar004
marked this pull request as ready for review
October 10, 2026 16:54
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
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.
Summary
Clears the four open high-severity CodeQL alerts on development (#55, #93, #94, #95). Each one is fixed at the cause. There are no
//nolintor// lgtmsuppressions.#93 Weak password hash, providers/aws/cognito/passwords.go
Data flow: the InitiateAuth / AdminInitiateAuth
PASSWORDparameter (auth.go) goes toverifyPassword, then to the old fallbacksha256.Sum256(salt + password).Cause: new hashes were already PBKDF2-SHA256 with a random 16-byte salt per user (
pbkdf2-sha256$<iterations>$<hex>). The fallback still checked the earlier bare SHA-256 format, so it ran a fast hash over the password on every sign-in.Fix: remove the fallback. A stored hash without the PBKDF2 prefix never matches, and sign-in returns
NotAuthorizedException, the same as a wrong password. The iteration count stays at 10,000 as a named constant, with a comment that it is deliberately low for an emulator (fast tests, no real secrets). Each hash stores its own iteration count, so changing the constant later keeps existing hashes valid.Compatibility: no release ever wrote the old format. It existed only on development between #1378 (Sep 27) and #1399 (Oct 3). v2.12.0 has no Cognito user records, and v2.13.0 already includes the PBKDF2 change. If a snapshot from a development build in that window holds such users, they can be recovered with
AdminSetUserPassword(aws cognito-idp admin-set-user-password --permanent).Tests: in
TestVerifyPassword, the old-format case now expects a rejection. The newTestSignInRejectsUnprefixedHashchecks that a user with a bare SHA-256 hash cannot sign in and thatAdminSetUserPasswordrecovers the account. Both fail on the old code.#94, #95 Integer narrowing, internal/vtl/eval.go
toIntData flow: 64-bit integers from
strconv.ParseInt(number literals, parsed JSON) and float64 values reachtoInt, which converts them withint(t).toIntfeeds:[a..b]get,set,charAt,substring, ...)Cause: there was no bounds check before narrowing. On 32-bit targets the int64 value truncates. On every target, converting NaN, infinity or an out-of-range float to int is undefined. For example,
[0..1e19]produced a 1001-item list on arm64 and something else on amd64.Fix:
toIntnow accepts only the int32 range, which is the Javaintthat Velocity uses for indexes, offsets and range bounds. NaN, infinities and out-of-range values are treated like any other non-integer argument: the result is null,setdoes nothing, and a bad range evaluates to null.listRemovenarrowed its int64 index withint(i)directly, so it now goes through the same check. Fractions still truncate.Tests:
TestToIntRangeandTestOutOfRangeNumbersInTemplatescover ranges, index reads and writes,get,set,remove,substringandcharAtwith values past int32, the int64 maximum, 1e19, NaN and infinity. The out-of-range cases fail on the old code.#55 Reflected XSS, features/vcr/vcr.go
Data flow: a request value echoed into an error body (for example
X-Amz-Targetin an ACMUnknownOperationException) goes towire.WriteJSONError, then through the Azure overlay'swriteJSONResponse, to the VCR recorder'scaptureWriter.Write, and finally to the live writer.Cause: at runtime the response was already safe. Upstream writers set a JSON/XML content type, and the recorder kept it (or set
application/octet-stream) and addedX-Content-Type-Options: nosniff. But the headers were set inWriteHeaderwhile the body was forwarded fromWrite. So the body write's safety relied on another method having run first, and CodeQL could not see a content type at the write.Fix:
WriteHeaderandWritenow both go through onesendmethod, the only path to the live writer. Before the headers go out,sendsets the content type (the handler's, or the default) andnosniffon that writer. Replay already works this way inwriteRecorded. The body is not escaped: SDKs parse these API responses byte for byte, so the recorder must not change them. Only the first status is forwarded, as net/http does.Tests:
TestRecordSetsSafeHeadersadds cases for an explicit status with no content type, atext/plaintype that should be kept, and a secondWriteHeadercall (the first one wins). The newTestRecordKeepsBodyAndStatuschecks that the recorder forwards and records several body writes and the status unchanged. Behaviour is unchanged here, so these tests guard against regressions and do not fail on the old code.Verification
go build ./...,go vetandgo test -raceon providers/aws/cognito, internal/vtl, features/vcr, providers/aws/apigateway, server/aws/apigateway, server/aws/cognito, server/serverkit and persist.golangci-lint --new-from-revon the touched packages reports 0 issues.go generateproduces no diff.cloudemu serve:NotAuthorizedException,admin-set-user-password, then the old password rejected and the new one accepted.get,substring,charAtand ranges. It renders empty values with HTTP 200 and no panic.<script>tag value, and a 404 with a reflected name) returnapplication/jsonandnosniffin both record and replay.