[PR #125] [MERGED] fix: validate required fields in OAuth state parser #173

New Issue

2026-06-06T22:09:33-04:00

yindo commented

2026-06-06 22:09:33 -04:00

📋 Pull Request Information

Original PR: https://github.com/vxcontrol/pentagi/pull/125
Author: @mason5052
Created: 2/22/2026
Status: ✅ Merged
Merged: 2/23/2026
Merged by: @asdek

Base: feature/project_improvements ← Head: fix/oauth-state-field-validation

📝 Commits (1)

50f1431 fix: validate required fields in OAuth state parser

📊 Changes

1 file changed (+16 additions, -1 deletions)

View changed files

📝 backend/pkg/server/services/auth.go (+16 -1)

📄 Description

Description of the Change

Problem

The parseState() function in auth.go accesses stateData["exp"] and stateData["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:

Missing exp: strconv.ParseInt("") fails with "invalid syntax" instead of clearly indicating the field is absent
Missing provider: empty string is used as a provider key, resulting in a confusing "provider not initialized" error in authLoginCallback()

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 (exp and provider) immediately after JSON unmarshalling in parseState(). 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:

Clear error messages for each specific missing field
Early validation before downstream usage
Consistent error handling pattern with the rest of the function

Ref: #101 (item 8)

Type of Change

🛡️ Security update

Areas Affected

Core Services (Frontend UI/Backend API)

Testing and Verification

Test Steps

Build the backend: cd backend && go build ./...
Run existing tests: go test ./pkg/server/services/...
Verify that valid OAuth flows still work normally
Verify that crafted states with missing exp or provider fields return clear error messages

Security 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

My code follows the project's coding standards
All new and existing tests pass
I have run go fmt and go vet (for Go code)

Security

I have considered security implications
Changes maintain or improve the security model

Compatibility

Changes are backward compatible

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/vxcontrol/pentagi/pull/125 **Author:** [@mason5052](https://github.com/mason5052) **Created:** 2/22/2026 **Status:** ✅ Merged **Merged:** 2/23/2026 **Merged by:** [@asdek](https://github.com/asdek) **Base:** `feature/project_improvements` ← **Head:** `fix/oauth-state-field-validation` --- ### 📝 Commits (1) - [`50f1431`](https://github.com/vxcontrol/pentagi/commit/50f14315011b2489b678cacd43e6eb377d31f39e) fix: validate required fields in OAuth state parser ### 📊 Changes **1 file changed** (+16 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `backend/pkg/server/services/auth.go` (+16 -1) </details> ### 📄 Description ### Description of the Change #### Problem The `parseState()` function in `auth.go` accesses `stateData["exp"]` and `stateData["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: - Missing `exp`: `strconv.ParseInt("")` fails with "invalid syntax" instead of clearly indicating the field is absent - Missing `provider`: empty string is used as a provider key, resulting in a confusing "provider not initialized" error in `authLoginCallback()` 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 (`exp` and `provider`) immediately after JSON unmarshalling in `parseState()`. 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: 1. Clear error messages for each specific missing field 2. Early validation before downstream usage 3. Consistent error handling pattern with the rest of the function Ref: #101 (item 8) ### Type of Change - [x] 🛡️ Security update ### Areas Affected - [x] Core Services (Frontend UI/Backend API) ### Testing and Verification #### Test Steps 1. Build the backend: `cd backend && go build ./...` 2. Run existing tests: `go test ./pkg/server/services/...` 3. Verify that valid OAuth flows still work normally 4. Verify that crafted states with missing `exp` or `provider` fields return clear error messages ### Security 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 - [x] My code follows the project's coding standards - [x] All new and existing tests pass - [x] I have run `go fmt` and `go vet` (for Go code) #### Security - [x] I have considered security implications - [x] Changes maintain or improve the security model #### Compatibility - [x] Changes are backward compatible --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

yindo added the pull-request label 2026-06-06 22:09:33 -04:00

yindo closed this issue

2026-06-06 22:09:33 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vxcontrol/pentagi#173