feat: adds jwt auth token middleware#121
Conversation
Signed-off-by: Amit Singh <singhamitch@outlook.com>
Signed-off-by: Amit Singh <singhamitch@outlook.com>
|
Warning Review limit reached
More reviews will be available in 24 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAuthentication migrated from API key to JWT tokens. New ChangesJWT Token Authentication Migration
Sequence Diagram(s)sequenceDiagram
participant TestSuite as BeforeSuite
participant AuthEndpoint as GET /auth/token
participant AuthHandler
participant AppServer
participant JWTAuth as AuthTokenMiddleware
participant Handler
TestSuite->>AuthEndpoint: GET /auth/token (Client-ID)
AuthEndpoint->>AuthHandler: routed
AuthHandler->>AuthHandler: validate Client-ID
AuthHandler->>AuthHandler: create & sign JWT (aud=client, iss=source-score, exp=4h)
AuthHandler->>TestSuite: 200 { "token": "<jwt>" }
Note over TestSuite: Subsequent requests include Authorization: Bearer <jwt>
TestSuite->>AppServer: GET /api/v1/claims (Authorization, Client-ID)
AppServer->>JWTAuth: incoming request
JWTAuth->>JWTAuth: parse & validate JWT (HS256, aud, iss, exp)
JWTAuth->>Handler: c.Next() on success
Handler->>TestSuite: 200 OK [claims]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Amit Singh <singhamitch@outlook.com>
7c0ee1c to
a92e328
Compare
Signed-off-by: Amit Singh <singhamitch@outlook.com>
…face Signed-off-by: Amit Singh <singhamitch@outlook.com>
Signed-off-by: Amit Singh <singhamitch@outlook.com>
6e18d28 to
bcfb944
Compare
Signed-off-by: Amit Singh <singhamitch@outlook.com>
bcfb944 to
659135e
Compare
1d6cc61 to
f73fe59
Compare
Signed-off-by: Amit Singh <singhamitch@outlook.com>
f73fe59 to
adca229
Compare
Signed-off-by: Amit Singh <singhamitch@outlook.com>
7e37ac5 to
0ec8cc6
Compare
Signed-off-by: Amit Singh <singhamitch@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
pkg/http/router.go (1)
82-85:VerifyClaimis a no-op and returns an empty200.It writes no response and doesn't invoke the handler, so callers get an empty success for individual claim verification. This appears to be the existing TODO state, just flagging it now that the signature is touched.
Want me to open an issue to track wiring (or removing) the individual claim-verification endpoint?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/http/router.go` around lines 82 - 85, The VerifyClaim handler is currently a no-op; wire it to call the claim validation logic and return a proper response instead of an empty 200. Specifically, invoke r.claimHandler.ValidateClaimByUriDigest(ctx, claimDigest) (or the existing claim handler method used for bulk verification) and translate its result/error into an HTTP response using the same pattern your other handlers use (e.g., ctx.JSON with the validation result on success and ctx.AbortWithStatusJSON or similar on error), or if individual verification is intentionally unsupported, remove the endpoint or return a clear 4xx/5xx (e.g., 501 Not Implemented) with an explanatory body. Ensure you reference VerifyClaim and r.claimHandler.ValidateClaimByUriDigest when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/source-score.yaml`:
- Around line 507-515: The OpenAPI spec currently defines only a ClientIDHeader
parameter (components.parameters.ClientIDHeader / name: Client-ID) but omits the
bearer Authorization security scheme; add a components.securitySchemes entry for
a bearer JWT (type: http, scheme: bearer, bearerFormat: JWT) and then apply it
to protected operations via either top-level components.security or
operation-level security arrays so that routes requiring JWT (all secured
endpoints except the explicit /auth/token path) document the required
Authorization: Bearer <token> header while leaving /auth/token unauthenticated
if intended.
- Around line 12-20: The /auth/token endpoint (operationId getAuthToken)
currently issues tokens based solely on the ClientIDHeader parameter — change
the contract and implementation so tokens are only minted after proper client
authentication: replace or augment ClientIDHeader with a strong credential
(e.g., Client-Secret) or require an OAuth2 / external IdP flow (e.g.,
client_credentials grant or authorization code) and validate the secret/IdP
assertion before creating JWTs; ensure the spec documents the required
credential/redirect and that server-side code verifies the credential/IdP token
and enforces audience/scopes before issuing any bearer token.
In `@pkg/conf/conf.go`:
- Line 21: The JwtSecret struct tag currently provides a hardcoded default which
is unsafe; update the JwtSecret field tag (symbol: JwtSecret) to remove
env-default:"default-secret-string" and mark the variable as required (e.g.,
replace the tag with `env:"JWT_SECRET" yaml:"JWT_SECRET" env-required:"true"` or
equivalent used elsewhere), ensuring no fallback default is present so the
service fails to start if JWT_SECRET is unset.
In `@pkg/handlers/auth.go`:
- Around line 22-40: GetAuthToken currently mints a JWT for any request with a
Client-ID header; add a credential verification step before signing: retrieve
the client's credential (e.g., Client-Secret header or mTLS info) and validate
it against the persisted client record using a method on AuthHandler (e.g.,
ah.ValidateClientSecret or ah.clientStore.LookupAndVerify), and if validation
fails return http.StatusUnauthorized; only on successful verification proceed to
create the token using ah.jwtSecret and the existing claims (ensure you
reference middleware.ClientIDHeader and middleware.TokenIssuer when validating
and issuing).
In `@pkg/middleware/auth.go`:
- Around line 41-44: The current check uses err.Error() which can panic if err
is nil and also leaks internal parse errors to clients; update the token
validation branch (the variables token, err and the call c.AbortWithStatusJSON)
to first separately handle err != nil and !token.Valid: if err != nil respond
with a generic unauthorized message (e.g., {"error":"invalid or expired token"})
without including err.Error(), and if err == nil but token.Valid is false
respond with the same generic message; ensure you do not call err.Error() when
err is nil and do not return raw error strings to the client.
- Around line 25-32: The Client-ID header check in the auth middleware writes a
400 with c.JSON but does not abort the Gin context, allowing downstream handlers
to run; update the block that checks clientID (using ClientIDHeader and the
gin.Context variable c in the auth middleware function) to call
c.AbortWithStatusJSON(...) with the same status and payload instead of c.JSON
and return, mirroring the Bearer-prefix branch so the request chain is halted
and no further handlers execute when the header is missing.
- Around line 34-40: The JWT parsing call accepts any signing algorithm because
the keyfunc does not check token.Method; update the jwt.ParseWithClaims
invocation (the call that currently passes jwt.WithAudience and jwt.WithIssuer)
to include jwt.WithValidMethods([]string{"HS256"}) to pin accepted algorithms to
HS256, and also add a simple sanity check inside the keyfunc used there
(jwt.ParseWithClaims's func(*jwt.Token)...) to assert token.Method.Alg() ==
"HS256" and return an error if not, ensuring tokens with other alg headers are
rejected.
---
Nitpick comments:
In `@pkg/http/router.go`:
- Around line 82-85: The VerifyClaim handler is currently a no-op; wire it to
call the claim validation logic and return a proper response instead of an empty
200. Specifically, invoke r.claimHandler.ValidateClaimByUriDigest(ctx,
claimDigest) (or the existing claim handler method used for bulk verification)
and translate its result/error into an HTTP response using the same pattern your
other handlers use (e.g., ctx.JSON with the validation result on success and
ctx.AbortWithStatusJSON or similar on error), or if individual verification is
intentionally unsupported, remove the endpoint or return a clear 4xx/5xx (e.g.,
501 Not Implemented) with an explanatory body. Ensure you reference VerifyClaim
and r.claimHandler.ValidateClaimByUriDigest when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e82ba4b-981e-4c05-b5c6-1e793f1f5353
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
acceptance/acceptance_suite_test.goacceptance/compose.yamlapi/source-score.yamlcmd/app/main.gogo.modpkg/api/server.gen.gopkg/conf/conf.gopkg/conf/conf.yamlpkg/conf/conf_test.gopkg/domain/claim/claim_repository.gopkg/domain/claim/claim_repository_test.gopkg/domain/claim/claim_service.gopkg/domain/claim/claim_service_test.gopkg/domain/claim/claimfakes/fake_claim_repository.gopkg/domain/claim/claimfakes/fake_claim_service.gopkg/handlers/auth.gopkg/handlers/claim.gopkg/handlers/swagger.gopkg/http/router.gopkg/middleware/apikey.gopkg/middleware/auth.go
💤 Files with no reviewable changes (2)
- pkg/middleware/apikey.go
- acceptance/compose.yaml
There was a problem hiding this comment.
12 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/handlers/auth.go">
<violation number="1" location="pkg/handlers/auth.go:23">
P0: [CRITICAL] `/auth/token` mints JWTs from an untrusted `Client-ID` header without authenticating the caller, enabling token issuance/impersonation for arbitrary clients.</violation>
</file>
<file name="pkg/http/router.go">
<violation number="1" location="pkg/http/router.go:34">
P1: Do not initialize JWT auth with a default/shared secret; require a non-default `JWT_SECRET` before wiring `AuthHandler`, or tokens can be forged.</violation>
</file>
<file name="pkg/conf/conf.go">
<violation number="1" location="pkg/conf/conf.go:21">
P0: [CRITICAL] Do not use a hardcoded default JWT secret. Require `JWT_SECRET` to be explicitly provided so the service fails fast instead of running with a predictable signing key.</violation>
</file>
<file name="pkg/middleware/auth.go">
<violation number="1" location="pkg/middleware/auth.go:20">
P2: [SUGGESTION] Match the `Authorization` scheme case-insensitively; valid lowercase/mixed-case `bearer` headers are currently rejected.</violation>
<violation number="2" location="pkg/middleware/auth.go:39">
P2: [CRITICAL] Restrict JWT parsing to expected signing method(s) using `jwt.WithValidMethods`; currently the parser is not pinned to HS256.</violation>
</file>
<file name="cmd/app/main.go">
<violation number="1" location="cmd/app/main.go:82">
P1: `Client-ID` is required by your API but missing from CORS `AllowHeaders`, which will block browser clients during preflight.</violation>
<violation number="2" location="cmd/app/main.go:102">
P0: Fail fast when `JWT_SECRET` is unset/default; using a known default secret makes JWT authentication forgeable.</violation>
</file>
<file name="api/source-score.yaml">
<violation number="1" location="api/source-score.yaml:13">
P2: Use `POST` for `/auth/token` instead of `GET` to avoid cache/read-only semantics on token issuance endpoints.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Signed-off-by: Amit Singh <singhamitch@outlook.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/handlers/auth.go">
<violation number="1" location="pkg/handlers/auth.go:23">
P0: [CRITICAL] `/auth/token` mints JWTs from an untrusted `Client-ID` header without authenticating the caller, enabling token issuance/impersonation for arbitrary clients.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Amit Singh <singhamitch@outlook.com>
6505eda to
dd32ee4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@acceptance/auth_test.go`:
- Line 18: The "Validation tests" block is marked pending via Ginkgo's XContext,
so those auth/JWT specs never run; open the block by replacing XContext with
Context (or Describe/When as appropriate) in acceptance/auth_test.go for the
"Validation tests" group and also scan the contained specs for any other pending
markers (XIt, XDescribe, Pending, PIt) and convert them to active equivalents so
the validation specs execute in CI.
- Around line 14-17: The protectedEndpoint and its assertion are being executed
at spec-tree construction inside Describe; move the url.JoinPath call and the
Expect(err).To(BeNil()) into a runtime hook such as BeforeEach so the
computation and assertion run during test execution. Locate the current
protectedEndpoint variable and the url.JoinPath(baseUrl, "/api/v1/claims") call
inside the Describe block and refactor them so protectedEndpoint is assigned in
a BeforeEach and the Expect(err).To(BeNil()) assertion is performed there (leave
Describe for declarations only).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d54f9579-0a9d-4a07-b047-bd0fd5d02df5
📒 Files selected for processing (8)
acceptance/acceptance_suite_test.goacceptance/auth_test.goacceptance/compose.yamlapi/source-score.yamlcmd/app/main.gopkg/api/server.gen.gopkg/conf/conf.gopkg/middleware/auth.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/middleware/auth.go
- cmd/app/main.go
- acceptance/acceptance_suite_test.go
Signed-off-by: Amit Singh <singhamitch@outlook.com>
Fixes: #118
Summary by CodeRabbit
New Features
Changes
Tests
Chores