feat: wire up opt-in security flag for Identity instances#451
feat: wire up opt-in security flag for Identity instances#451VaibhavAcharya wants to merge 2 commits into
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a SecurityConfiguration type (Strict bool) and a GlobalConfiguration flag NewInstancesSecureByDefault; embeds Security into per-instance Configuration. CreateInstance now, when NewInstancesSecureByDefault is enabled and a BaseConfig is provided, forces BaseConfig.Security.Strict = true before persisting. The Settings API response gains a security_strict field populated from config.Security.Strict. Tests and example.env updated to cover defaults and flag-driven behavior. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
VaibhavAcharya
force-pushed
the
vaibhavacharya/identity-security-opt-in
branch
from
93b64c1 to
400e8bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
conf/configuration.go (1)
131-133: ⚡ Quick winAlign redirect allowlist comment with actual matcher semantics.
The comment says “exact-match allowlist,” but
api/external.go:isAllowedRedirectURIallows path-segment prefix matches (e.g.,/authmatching/auth/cb). Please update this wording to avoid security-policy misconfiguration by operators.🤖 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 `@conf/configuration.go` around lines 131 - 133, The comment for AllowedRedirectURIs is misleading (it says "exact-match allowlist") while the actual matcher is implemented by isAllowedRedirectURI and permits path-segment prefix matches (e.g., a configured "/auth" allows "/auth/cb"); update the comment on AllowedRedirectURIs to accurately describe this semantics—state that entries are matched by scheme, host (or host-only) and by path prefix matching on path segments (not strict exact path equality), and note any normalization rules (trailing slashes, default ports) that isAllowedRedirectURI applies so operators won't misconfigure the allowlist.api/password_test.go (1)
23-29: ⚡ Quick winAdd a multibyte-password test case to lock in character-length behavior.
Please add a strict-mode case with non-ASCII characters (e.g., emojis/CJK) so the test suite enforces character-count semantics and prevents byte-length regressions.
🤖 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 `@api/password_test.go` around lines 23 - 29, Add a strict-mode test row to the test cases slice in api/password_test.go that uses the strict identifier and a multibyte password value (exactly eight Unicode characters, e.g., eight CJK/emoji characters) and set the expected result to false so the test asserts character-count (not byte-count) behavior; place it alongside the existing strict cases (near the "strict at min" / "strict above min" entries) to lock in multibyte character-length semantics.api/instance_test.go (1)
78-160: ⚡ Quick winAdd regression coverage for config-less create under secure-by-default.
Please add a test with
NewInstancesSecureByDefault=trueand no"config"field in the create payload, then assert the instance remains config-less (no forced strict config). This locks in the anti-lockout behavior documented in the handler.🤖 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 `@api/instance_test.go` around lines 78 - 160, Add a new test method on InstanceTestSuite (e.g., TestCreate_SecureByDefaultKeepsConfigLess) that sets ts.API.config.NewInstancesSecureByDefault = true (restore prev with defer), creates a POST /instances payload that includes only "uuid" (no "config" key) using a fresh uuid and operatorToken, calls ts.API.handler.ServeHTTP and asserts StatusCreated, then load the instance with models.GetInstanceByUUID and assert that i.BaseConfig is nil (or that Security.Strict is not present) to ensure a config-less create remains config-less under secure-by-default; follow the same request/encoding/assert patterns used in TestCreate_SecureByDefaultFlipsEnabled to match style.
🤖 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/password.go`:
- Around line 17-21: The password length check in validatePassword currently
uses len(password) (byte count) which miscounts UTF-8 characters; change the
check to use utf8.RuneCountInString(password) from the unicode/utf8 package so
the policy enforces character (rune) length correctly, update imports to include
"unicode/utf8" if missing, and keep the same unprocessableEntityError message
using config.Security.MinPasswordLength.
In `@api/settings_test.go`:
- Around line 71-74: The test currently decodes into resp (type Settings) so a
missing security_strict key silently becomes false; change the test to first
decode w.Body into a map[string]json.RawMessage (or map[string]interface{}) and
assert that the key "security_strict" exists, then decode into Settings (resp)
and assert resp.SecurityStrict is false; reference the existing symbols resp,
Settings, SecurityStrict, w.Body and json.NewDecoder to locate the change.
---
Nitpick comments:
In `@api/instance_test.go`:
- Around line 78-160: Add a new test method on InstanceTestSuite (e.g.,
TestCreate_SecureByDefaultKeepsConfigLess) that sets
ts.API.config.NewInstancesSecureByDefault = true (restore prev with defer),
creates a POST /instances payload that includes only "uuid" (no "config" key)
using a fresh uuid and operatorToken, calls ts.API.handler.ServeHTTP and asserts
StatusCreated, then load the instance with models.GetInstanceByUUID and assert
that i.BaseConfig is nil (or that Security.Strict is not present) to ensure a
config-less create remains config-less under secure-by-default; follow the same
request/encoding/assert patterns used in TestCreate_SecureByDefaultFlipsEnabled
to match style.
In `@api/password_test.go`:
- Around line 23-29: Add a strict-mode test row to the test cases slice in
api/password_test.go that uses the strict identifier and a multibyte password
value (exactly eight Unicode characters, e.g., eight CJK/emoji characters) and
set the expected result to false so the test asserts character-count (not
byte-count) behavior; place it alongside the existing strict cases (near the
"strict at min" / "strict above min" entries) to lock in multibyte
character-length semantics.
In `@conf/configuration.go`:
- Around line 131-133: The comment for AllowedRedirectURIs is misleading (it
says "exact-match allowlist") while the actual matcher is implemented by
isAllowedRedirectURI and permits path-segment prefix matches (e.g., a configured
"/auth" allows "/auth/cb"); update the comment on AllowedRedirectURIs to
accurately describe this semantics—state that entries are matched by scheme,
host (or host-only) and by path prefix matching on path segments (not strict
exact path equality), and note any normalization rules (trailing slashes,
default ports) that isAllowedRedirectURI applies so operators won't misconfigure
the allowlist.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b97bd60f-5c32-4763-9cd4-c8a11bdc9ece
📒 Files selected for processing (20)
api/admin.goapi/api.goapi/api_test.goapi/context.goapi/external.goapi/external_test.goapi/instance.goapi/instance_test.goapi/middleware.goapi/password.goapi/password_test.goapi/settings.goapi/settings_test.goapi/signup.goapi/signup_test.goapi/user.goapi/verify.goconf/configuration.goconf/configuration_test.goexample.env
VaibhavAcharya
force-pushed
the
vaibhavacharya/identity-security-opt-in
branch
from
400e8bc to
236ab3b
Compare
VaibhavAcharya
force-pushed
the
vaibhavacharya/identity-security-opt-in
branch
from
236ab3b to
fca8b32
Compare
VaibhavAcharya
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/instance.go`:
- Around line 77-83: The code treats SecurityConfiguration.Strict (bool) the
same for “unset” and explicit false, so update the model and handling to allow
an explicit opt-out: change SecurityConfiguration.Strict from bool to *bool in
conf/configuration.go, update all usages (notably the POST /instances logic that
references params.BaseConfig.Security.Strict and the NewInstancesSecureByDefault
check) to test for nil (treat nil as unset) and only set true when Strict == nil
and NewInstancesSecureByDefault is true, and add a unit test covering explicit
`"security": {"strict": false}` to ensure callers can opt out; alternatively, if
you prefer doc-only fix, update the wording in conf/configuration.go to clearly
state that NewInstancesSecureByDefault overrides both omitted and explicit false
values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd968bac-8bba-4d30-9e15-643a0394c92d
📒 Files selected for processing (7)
api/instance.goapi/instance_test.goapi/settings.goapi/settings_test.goconf/configuration.goconf/configuration_test.goexample.env
🚧 Files skipped from review as they are similar to previous changes (4)
- example.env
- api/settings.go
- api/settings_test.go
- api/instance_test.go
VaibhavAcharya
force-pushed
the
vaibhavacharya/identity-security-opt-in
branch
from
c20c091 to
d16a894
Compare
1 participant
- Summary
First of four PRs splitting the Identity security hardening work (originally this PR's combined diff) into reviewable pieces: this one wires up the opt-in mechanism, then one follow-up per issue adds a behavior gated on it.
Some of the remaining Identity security improvements are breaking changes for existing GoTrue clients, so we cannot turn them on globally. This PR adds a per-instance opt-in flag (
Security.Strict) that the stricter behaviors hang off. Existing instances keep their current behavior since the flag defaults off. New instances can be made strict by default through a global flag (GOTRUE_NEW_INSTANCES_SECURE_BY_DEFAULT) once the control plane is ready to populate the per-instance security config.Scaffolding only, no behavior change on its own:
SecurityConfigurationwith theStrictopt-in flagGOTRUE_NEW_INSTANCES_SECURE_BY_DEFAULTthat flipsStrictfor new instances created with a config (config-less instances stay permissive to avoid locking themselves out)security_strictexposed onGET /settingsThe gated behaviors land in follow-ups: minimum password length (EX-1871), OAuth redirect URI allowlist (EX-1884), and CORS origin allowlist (EX-1883).
- Test plan
go build ./...andgo vet ./...pass.go test -run 'TestSecurityConfigurationDefaults|TestNewInstancesSecureByDefault' ./conf/...passes.TestSettings_SecurityStrict*,InstanceTestSuite/TestCreate_SecureByDefault*) need MySQL (hack/mysqld.sh); rungo test ./api/...against it.- Description for the changelog
Add a per-instance opt-in flag (
Security.Strict) and a global secure-by-default flag for Identity instances, ahead of the stricter security behaviors that build on it.Link EX-1871
Link EX-1884
Link EX-1883