fix: validate required fields in OAuth state parser#125
Merged
asdek merged 1 commit intovxcontrol:feature/project_improvementsfrom Feb 23, 2026
Merged
Conversation
Add explicit existence checks for 'exp' and 'provider' fields in parseState() before accessing them. Previously, missing fields would produce misleading error messages (e.g., strconv.ParseInt on empty string gives "invalid syntax" rather than indicating the field is missing). An empty provider string causes a confusing "not initialized" error downstream in authLoginCallback(). This provides defense-in-depth validation with clear error messages for each missing required field. Ref: vxcontrol#101 (item 8) Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
There was a problem hiding this comment.
Pull request overview
Hardens the OAuth state parsing logic in AuthService.parseState() by validating required fields up-front, improving error clarity and reducing confusing downstream failures during OAuth callbacks (defense-in-depth per issue #101 item 8).
Changes:
- Add explicit presence/emptiness validation for the
expfield before parsing it as an integer. - Add explicit presence validation for the
providerfield before it’s used by the OAuth callback handler.
Comments suppressed due to low confidence (1)
backend/pkg/server/services/auth.go:649
provideris only checked for key presence, not for an empty value. A crafted/buggy state payload with{"provider":""}will still pass validation and later hitauthLoginCallback()with an empty provider, producing the same confusing "provider not initialized" error this PR aims to avoid. Consider validatingprovidersimilarly toexp(check existence and non-empty).
if _, ok := stateData["provider"]; !ok {
err := fmt.Errorf("missing required field: provider")
logger.FromContext(c).WithError(err).Errorf("error on validating state data")
response.Error(c, response.ErrAuthInvalidAuthorizationState, err)
return nil, err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
707007b
into
vxcontrol:feature/project_improvements
3 of 4 checks passed
Contributor
|
LGTM thank you for contribution! |
33 tasks
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.
Description of the Change
Problem
The
parseState()function inauth.goaccessesstateData["exp"]andstateData["provider"]without first checking whether these keys exist in the map. While Go map access on missing keys returns the zero value (empty string) rather than panicking, this produces misleading error messages:exp:strconv.ParseInt("")fails with "invalid syntax" instead of clearly indicating the field is absentprovider: empty string is used as a provider key, resulting in a confusing "provider not initialized" error inauthLoginCallback()If the HMAC signing key were ever compromised or a signing bug introduced, crafted state payloads with missing fields would produce confusing errors that complicate incident response.
Solution
Add explicit existence checks for required fields (
expandprovider) immediately after JSON unmarshalling inparseState(). Each missing field now returns a clear, specific error message (e.g., "missing required field: exp") with proper logging.This is a defense-in-depth measure that ensures:
Ref: #101 (item 8)
Type of Change
Areas Affected
Testing and Verification
Test Steps
cd backend && go build ./...go test ./pkg/server/services/...exporproviderfields return clear error messagesSecurity Considerations
This change hardens the OAuth state parser against malformed state data. While the state is HMAC-signed (preventing tampering under normal conditions), this defense-in-depth validation ensures clear error handling even if the signing mechanism is bypassed.
Checklist
Code Quality
go fmtandgo vet(for Go code)Security
Compatibility