Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughLicense URL updated to https; OpenAPI adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.golangci.yml (2)
396-398:allow-unused: trueweakens nolint hygiene.This means stale
//nolintdirectives won't be caught. If this is a temporary measure during themodernizelinter rollout, consider reverting it once the codebase stabilizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 396 - 398, The golangci-lint setting allow-unused: true in the .golangci.yml weakens nolint hygiene by ignoring stale //nolint directives; change allow-unused to false (or remove the override) so the linter enforces that all //nolint comments are actually used, and if this was temporary for the modernize rollout, add a todo comment in the file noting to revert to false when rollout stabilizes.
342-347: Consider using targeted//nolint:gosecdirectives instead of global G117 exclusion.G117 catches potential secret exposure via JSON marshaling. However, this linter produces false positives on metric constant names containing "token" (e.g.,
jwt_tokens_issued_total). The codebase already uses targeted//nolint:goseccomments with explanations for these false positives (seeinternal/sms-gateway/jwt/metrics.go), and sensitive data like passwords and tokens are appropriately handled without JSON marshaling.Rather than a global G117 exclusion, consider applying targeted suppressions to the specific constants that trigger false positives, which aligns with your existing practice elsewhere in the codebase.
G704 (SSRF taint analysis) exclusion is reasonable given the server-side nature of the application.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 342 - 347, The global suppression of gosec rule G117 in .golangci.yml is too broad; instead remove G117 from the top-level excludes and add targeted //nolint:gosec comments with explanations on the specific offending identifiers (e.g., metric constants like jwt_tokens_issued_total) similar to the existing pattern in internal/sms-gateway/jwt/metrics.go; keep G704 excluded as-is. Locate the metric constant declarations that triggered G117, add a //nolint:gosec comment with a short rationale (e.g., "metric name contains 'token' but does not expose secrets via JSON"), and update the linter config to delete the global G117 entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.golangci.yml:
- Around line 396-398: The golangci-lint setting allow-unused: true in the
.golangci.yml weakens nolint hygiene by ignoring stale //nolint directives;
change allow-unused to false (or remove the override) so the linter enforces
that all //nolint comments are actually used, and if this was temporary for the
modernize rollout, add a todo comment in the file noting to revert to false when
rollout stabilizes.
- Around line 342-347: The global suppression of gosec rule G117 in
.golangci.yml is too broad; instead remove G117 from the top-level excludes and
add targeted //nolint:gosec comments with explanations on the specific offending
identifiers (e.g., metric constants like jwt_tokens_issued_total) similar to the
existing pattern in internal/sms-gateway/jwt/metrics.go; keep G704 excluded
as-is. Locate the metric constant declarations that triggered G117, add a
//nolint:gosec comment with a short rationale (e.g., "metric name contains
'token' but does not expose secrets via JSON"), and update the linter config to
delete the global G117 entry.
capcom6
force-pushed
the
docs/add-501-for-auth
branch
from
81a311a to
459d266
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sms-gateway/handlers/thirdparty/auth.go (1)
84-96:⚠️ Potential issue | 🟡 Minor
deleteTokendescription is missing the "Not available in Local Server mode" caveat.
postToken(line 44) documents the condition under which 501 is returned;deleteTokenlists the same 501 failure but omits the explanation. Both endpoints flow through the sameerrorHandlerthat mapsjwt.ErrDisabled→ 501, so the caveat applies equally here.📝 Proposed fix
-// `@Description` Revoke access token with specified jti +// `@Description` Revoke access token with specified jti\r\n\r\n*Not available in Local Server mode*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/handlers/thirdparty/auth.go` around lines 84 - 96, Update the swagger comment block for the deleteToken endpoint to include the same caveat as postToken: document that 501 "Not implemented" is returned when the server is running in Local Server mode (the error maps from jwt.ErrDisabled via errorHandler). Locate the deleteToken comment block in internal/sms-gateway/handlers/thirdparty/auth.go and add the explanatory text used in postToken (or equivalent wording) next to the 501 Failure entry so the API docs clearly state the 501 condition.
🧹 Nitpick comments (1)
api/local.http (1)
108-111: Optional:DELETE /webhooksstill uses Basic auth while GET/POST now use Bearer.Lines 94, 100 were migrated to Bearer, but the sibling DELETE at line 111 was not. If the intent is to fully demonstrate Bearer auth for the webhooks group, this endpoint should be updated too.
📝 Proposed fix
### DELETE {{localUrl}}/webhooks/LreFUt-Z3sSq0JufY9uWB HTTP/1.1 -Authorization: Basic {{localCredentials}} +# Authorization: Basic {{localCredentials}} +Authorization: Bearer {{localToken}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/local.http` around lines 108 - 111, The DELETE webhook example still uses Basic auth; change its Authorization header to use the same Bearer token style as the GET/POST examples by replacing "Authorization: Basic {{localCredentials}}" with "Authorization: Bearer {{localCredentials}}" (or the matching bearer variable used in the other webhooks requests) so the DELETE {{localUrl}}/webhooks/LreFUt-Z3sSq0JufY9uWB request is consistent with the migrated Bearer auth examples (see the surrounding GET/POST webhook requests for the exact bearer variable name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/sms-gateway/handlers/thirdparty/auth.go`:
- Around line 84-96: Update the swagger comment block for the deleteToken
endpoint to include the same caveat as postToken: document that 501 "Not
implemented" is returned when the server is running in Local Server mode (the
error maps from jwt.ErrDisabled via errorHandler). Locate the deleteToken
comment block in internal/sms-gateway/handlers/thirdparty/auth.go and add the
explanatory text used in postToken (or equivalent wording) next to the 501
Failure entry so the API docs clearly state the 501 condition.
---
Nitpick comments:
In `@api/local.http`:
- Around line 108-111: The DELETE webhook example still uses Basic auth; change
its Authorization header to use the same Bearer token style as the GET/POST
examples by replacing "Authorization: Basic {{localCredentials}}" with
"Authorization: Bearer {{localCredentials}}" (or the matching bearer variable
used in the other webhooks requests) so the DELETE
{{localUrl}}/webhooks/LreFUt-Z3sSq0JufY9uWB request is consistent with the
migrated Bearer auth examples (see the surrounding GET/POST webhook requests for
the exact bearer variable name).
4a598ac to
6d0dd74
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/sms-gateway/openapi/docs.go (1)
1737-1743:notification_channelinline enum is inconsistent with the rest of the schema and will lack named constants in generated client SDKs.Every other enum-typed settings field (
limit_period,processing_order,sim_selection_mode, etc.) references a dedicated top-level definition that carriesx-enum-varnames.notification_channeluses an inline enum, which means:
- Code generators won't produce named constants (e.g.,
NotificationChannelAuto,NotificationChannelSseOnly).- The enum values won't be reusable if referenced elsewhere.
Since this is a generated file, the fix belongs in the source Go struct/swag annotation — introduce a named
NotificationChanneltype and annotate accordingly, then regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sms-gateway/openapi/docs.go` around lines 1737 - 1743, The inline enum for "notification_channel" should be replaced by a named enum type so generated SDKs get proper constants: introduce a top-level NotificationChannel enum type in the Go sources (e.g., type NotificationChannel string) and update the struct field that currently renders as "notification_channel" to use that named type, then add the appropriate swagger/swag annotations (including x-enum-varnames) on the NotificationChannel definition so the OpenAPI generator emits a top-level schema with enum and x-enum-varnames instead of an inline enum; regenerate the docs so the OpenAPI snippet for "notification_channel" references NotificationChannel rather than embedding the enum values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/sms-gateway/openapi/docs.go`:
- Around line 1737-1743: The inline enum for "notification_channel" should be
replaced by a named enum type so generated SDKs get proper constants: introduce
a top-level NotificationChannel enum type in the Go sources (e.g., type
NotificationChannel string) and update the struct field that currently renders
as "notification_channel" to use that named type, then add the appropriate
swagger/swag annotations (including x-enum-varnames) on the NotificationChannel
definition so the OpenAPI generator emits a top-level schema with enum and
x-enum-varnames instead of an inline enum; regenerate the docs so the OpenAPI
snippet for "notification_channel" references NotificationChannel rather than
embedding the enum values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sms-gateway/handlers/thirdparty/auth.gointernal/sms-gateway/openapi/docs.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/sms-gateway/handlers/thirdparty/auth.go
1 participant
Summary by CodeRabbit
Documentation
New Features