Describe the Bug
CodeRabbit flagged several correctness/security bugs on PR #517 (Portal Backend restructure). Verified against the pre-move portal-backend/ code and confirmed each is a pre-existing issue carried over as-is by the pure structural move, not something introduced by the restructure, so they're tracked here instead of being fixed in that PR.
internal/pb/v1/middleware/cors.go:67-81 — CORS middleware allows AllowedOrigins: ["*"] together with AllowCredentials: true. Browsers reject Access-Control-Allow-Origin: * on credentialed responses, so authenticated portal requests fail if this combination is ever configured. Should reject/invalidate this combination and require explicit trusted origins when credentials are enabled.internal/pb/v1/models/authentication.go:244 — The JWT role-filtering warning logs claims.IdpUserID (an end-user identifier) via slog.Warn. Should log only non-identifying metadata (e.g. invalid-role count), with invalidRoles redacted/bounded.internal/pb/idp/asgardeo/client.go:27 — oauthConfig.Client(context.Background()) is used for token acquisition, ignoring the request's context. A blocked/slow token endpoint won't respect request cancellation/timeout.internal/pb/v1/middleware/jwt_auth_concurrency_test.go:135-152 — hasInitialKey/hasNewKey are computed via != nil against map values that are always stored as nil, so both are always false. The inconsistency check this test exists to run is unreachable — the test currently can't catch the atomicity bug it targets. Needs comma-ok map lookups instead.internal/pb/v1/models/base_test.go:29-30,63-64 — AutoMigrate/DropTable errors are discarded. A failed migration can produce misleading downstream test failures instead of failing fast at setup.internal/pb/v1/models/base_test.go:76-89 — The update-hook test pre-assigns UpdatedAt before Save, then asserts against that same value — so it can pass even if the BeforeUpdate lifecycle hook isn't actually setting it.internal/pb/v1/database_test.go:11-49 — DB_* environment variables aren't isolated via t.Setenv, so TestNewDatabaseConfig can fail (and TestNewDatabaseConfig_WithEnvVars's manual cleanup can leak) depending on the ambient environment.
To Reproduce
Code-review findings (not user-facing repro steps). See file/line references above; each includes a proposed fix in the original CodeRabbit review on PR #517.
Expected Behavior
Each item above behaves/tests correctly per the description; see PR #517's CodeRabbit review comments for the proposed diffs.
Version
commit 25d8eb1 (pre-move equivalents unchanged since the module's original portal-backend/ implementation)
Additional Context
Surfaced by CodeRabbit's review of #517, which restructured portal-backend/ into cmd/pb + internal/pb as a pure move (each flagged file diffs to 0 changes against its pre-move original). Filed separately so they can be fixed on their own merits rather than bundled into the restructuring PR.
Describe the Bug
CodeRabbit flagged several correctness/security bugs on PR #517 (Portal Backend restructure). Verified against the pre-move
portal-backend/code and confirmed each is a pre-existing issue carried over as-is by the pure structural move, not something introduced by the restructure, so they're tracked here instead of being fixed in that PR.internal/pb/v1/middleware/cors.go:67-81— CORS middleware allowsAllowedOrigins: ["*"]together withAllowCredentials: true. Browsers rejectAccess-Control-Allow-Origin: *on credentialed responses, so authenticated portal requests fail if this combination is ever configured. Should reject/invalidate this combination and require explicit trusted origins when credentials are enabled.internal/pb/v1/models/authentication.go:244— The JWT role-filtering warning logsclaims.IdpUserID(an end-user identifier) viaslog.Warn. Should log only non-identifying metadata (e.g. invalid-role count), withinvalidRolesredacted/bounded.internal/pb/idp/asgardeo/client.go:27—oauthConfig.Client(context.Background())is used for token acquisition, ignoring the request's context. A blocked/slow token endpoint won't respect request cancellation/timeout.internal/pb/v1/middleware/jwt_auth_concurrency_test.go:135-152—hasInitialKey/hasNewKeyare computed via!= nilagainst map values that are always stored asnil, so both are alwaysfalse. The inconsistency check this test exists to run is unreachable — the test currently can't catch the atomicity bug it targets. Needs comma-ok map lookups instead.internal/pb/v1/models/base_test.go:29-30,63-64—AutoMigrate/DropTableerrors are discarded. A failed migration can produce misleading downstream test failures instead of failing fast at setup.internal/pb/v1/models/base_test.go:76-89— The update-hook test pre-assignsUpdatedAtbeforeSave, then asserts against that same value — so it can pass even if theBeforeUpdatelifecycle hook isn't actually setting it.internal/pb/v1/database_test.go:11-49—DB_*environment variables aren't isolated viat.Setenv, soTestNewDatabaseConfigcan fail (andTestNewDatabaseConfig_WithEnvVars's manual cleanup can leak) depending on the ambient environment.To Reproduce
Code-review findings (not user-facing repro steps). See file/line references above; each includes a proposed fix in the original CodeRabbit review on PR #517.
Expected Behavior
Each item above behaves/tests correctly per the description; see PR #517's CodeRabbit review comments for the proposed diffs.
Version
commit 25d8eb1 (pre-move equivalents unchanged since the module's original portal-backend/ implementation)
Additional Context
Surfaced by CodeRabbit's review of #517, which restructured
portal-backend/intocmd/pb+internal/pbas a pure move (each flagged file diffs to 0 changes against its pre-move original). Filed separately so they can be fixed on their own merits rather than bundled into the restructuring PR.