feat: add device profiles#950
Open
wizzomafizzo wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis pull request implements a complete device profiles feature: passwordless preference snapshots with optional per-profile PIN protection, profile-scoped playtime limits, API endpoints for CRUD and switching, NFC card-based activation bypassing PIN checks, and a configuration gate requiring active profiles before media launch. The system attributes media history to profiles at recording time and uses a pluggable limits provider to layer profile overrides over global playtime settings. ChangesDevice Profiles Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
pkg/service/profiles/resolver.go (1)
94-103: 💤 Low valueConsider explicitly handling "0" as a special case for clarity.
The code currently relies on
time.ParseDurationerror handling to interpret the string"0"(without a unit suffix) as unlimited. While this works defensively, explicitly handling"0"would make the intent clearer and align with the documented behavior in line 35.♻️ Proposed refactor for explicit "0" handling
func parseLimit(s string) time.Duration { - if s == "" { + if s == "" || s == "0" { return 0 } d, err := time.ParseDuration(s) if err != nil { return 0 } return d }🤖 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/service/profiles/resolver.go` around lines 94 - 103, The parseLimit function currently relies on time.ParseDuration to treat the string "0" as an invalid duration and return 0; update parseLimit to explicitly handle the literal "0" (or possibly "0s") as a special case by checking if s == "0" (and/or s == "0s") at the top and returning 0 immediately, then fall through to time.ParseDuration for other values and keep the existing error handling that returns 0 on parse failure; this clarifies intent in parseLimit and aligns with the documented behavior.pkg/service/profiles/profiles_test.go (1)
108-131: ⚡ Quick winRate limiting test doesn't cover the TOCTOU race.
TestActivateByID_RateLimitedverifies single-threaded rate limiting behavior but won't catch the race condition flagged inprofiles.go#L311-L346. The race only manifests when multiple goroutines callActivateByIDconcurrently for the same profile.📝 Example concurrent test to catch the race
func TestActivateByID_ConcurrentRateLimiting(t *testing.T) { t.Parallel() svc, mockDB, _ := newTestService(t) mockDB.On("GetProfile", "profile-1").Return(pinProfile(t, "1234"), nil) var wg sync.WaitGroup errors := make(chan error, 10) // Fire 10 concurrent wrong-PIN attempts for range 10 { wg.Add(1) go func() { defer wg.Done() _, err := svc.ActivateByID("profile-1", "9999") errors <- err }() } wg.Wait() close(errors) incorrectCount := 0 rateLimitedCount := 0 for err := range errors { if errors.Is(err, ErrPINIncorrect) { incorrectCount++ } else if errors.Is(err, ErrPINRateLimited) { rateLimitedCount++ } } // After fixing the race, we expect at most pinAttemptLimit incorrect, // rest should be rate-limited assert.LessOrEqual(t, incorrectCount, pinAttemptLimit) assert.GreaterOrEqual(t, rateLimitedCount, 10-pinAttemptLimit) }🤖 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/service/profiles/profiles_test.go` around lines 108 - 131, Add a concurrent test that reproduces the TOCTOU race by invoking svc.ActivateByID concurrently for the same profile: create TestActivateByID_ConcurrentRateLimiting (or augment TestActivateByID_RateLimited) to spin up multiple goroutines that call ActivateByID with a wrong PIN while mocking GetProfile to return the known PIN, collect errors into a channel, then assert that at most pinAttemptLimit calls return ErrPINIncorrect and the remainder return ErrPINRateLimited; this will surface the race in profiles.go handling around ActivateByID and the pin attempt window logic so you can fix the synchronization there.
🤖 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 `@docs/api/methods.md`:
- Line 2458: The "Profile object" heading is using too many hashes (shown as
"##### Profile object") and jumps from the previous "##" level; update that
heading to the correct level to follow the surrounding section hierarchy (e.g.,
change "##### Profile object" to "### Profile object" or the appropriate level
used by the nearby "Profiles" section) so the markdown outline/navigation is
consistent; locate the literal "Profile object" heading in docs/api/methods.md
and adjust the hash count to match the surrounding headings.
In `@docs/api/notifications.md`:
- Around line 420-423: The docs declare the notifications parameter "profile"
with type "object" but its description says it can be null; update the parameter
type to explicitly show it is nullable (use "object | null") so the contract is
unambiguous—edit the table row for the "profile" field in the notifications API
docs and change the Type column from "object" to "object | null" while keeping
the existing description.
In `@pkg/api/models/params.go`:
- Around line 183-190: The handler currently accepts both ProfileID and SwitchID
and silently prefers ProfileID; update the HandleProfilesSwitch request
validation to enforce mutual exclusivity per SwitchProfileParams: if both params
are non-nil/non-empty return a 400 error (with a clear message like "provide
exactly one of profileId or switchId") instead of proceeding, and add a
unit/integration test that posts a payload containing both profileId and
switchId and asserts the handler responds with the expected bad-request error;
touch the SwitchProfileParams handling branch in HandleProfilesSwitch to perform
this check early before any branch-specific logic.
In `@pkg/service/profiles/pin.go`:
- Around line 61-64: The pbkdf2.Key call is incorrect: use the signature
Key(password []byte, salt []byte, iter int, keyLen int, h func() hash.Hash)
[]byte; convert the pin string to []byte and pass parameters in that order
(password, salt, pinIterations, pinKeyLen, sha256.New), capture only the
returned []byte (no err) e.g. key := pbkdf2.Key([]byte(pin), salt,
pinIterations, pinKeyLen, sha256.New), then proceed with whatever
encoding/return the derived key instead of checking a non-existent error; update
any references in the same function (e.g., where key, err were used) to reflect
the single returned value.
- Around line 94-97: The pbkdf2.Key call in pkg/service/profiles/pin.go is using
the wrong parameter order and types and incorrectly expects an error; update the
call to use pbkdf2.Key with password as []byte(pin), salt as the []byte salt,
iterations as the iter int, key length as len(expected), and sha256.New as the
final parameter (pbkdf2.Key returns only []byte), then remove the err handling
around that call and adjust subsequent logic that compares the resulting key
(e.g., where `expected` is used) to use the returned []byte key.
In `@pkg/service/profiles/profiles.go`:
- Around line 311-346: The checkPIN function suffers a TOCTOU race: it computes
recent from s.pinAttempts under s.mu, releases the lock to call VerifyPIN, then
later reuses the stale recent when appending, causing lost attempts; fix by
re-loading and re-filtering the current attempts after reacquiring the lock
(i.e., after VerifyPIN and before appending) or by keeping the lock across the
VerifyPIN call if acceptable; specifically, in checkPIN re-acquire s.mu and
recompute attempts := s.pinAttempts[p.ProfileID] and recent = filtered attempts
using pinAttemptWindow before doing s.pinAttempts[p.ProfileID] = append(recent,
now), using the same pinAttemptLimit/pinAttemptWindow variables to enforce rate
limiting correctly.
---
Nitpick comments:
In `@pkg/service/profiles/profiles_test.go`:
- Around line 108-131: Add a concurrent test that reproduces the TOCTOU race by
invoking svc.ActivateByID concurrently for the same profile: create
TestActivateByID_ConcurrentRateLimiting (or augment
TestActivateByID_RateLimited) to spin up multiple goroutines that call
ActivateByID with a wrong PIN while mocking GetProfile to return the known PIN,
collect errors into a channel, then assert that at most pinAttemptLimit calls
return ErrPINIncorrect and the remainder return ErrPINRateLimited; this will
surface the race in profiles.go handling around ActivateByID and the pin attempt
window logic so you can fix the synchronization there.
In `@pkg/service/profiles/resolver.go`:
- Around line 94-103: The parseLimit function currently relies on
time.ParseDuration to treat the string "0" as an invalid duration and return 0;
update parseLimit to explicitly handle the literal "0" (or possibly "0s") as a
special case by checking if s == "0" (and/or s == "0s") at the top and returning
0 immediately, then fall through to time.ParseDuration for other values and keep
the existing error handling that returns 0 on parse failure; this clarifies
intent in parseLimit and aligns with the documented behavior.
🪄 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
Run ID: 0d7327b2-dfc7-41e1-851b-52a67c72b3db
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
docs/ARCHITECTURE.mddocs/api/methods.mddocs/api/notifications.mdgo.modpkg/api/methods/profiles.gopkg/api/methods/profiles_test.gopkg/api/methods/settings.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/requests/requests.gopkg/api/models/responses.gopkg/api/notifications/notifications.gopkg/api/server.gopkg/api/server_post_test.gopkg/api/server_startup_test.gopkg/api/server_ws_e2e_test.gopkg/api/ws_dispatcher_test.gopkg/config/config.gopkg/config/configprofiles.gopkg/config/configprofiles_test.gopkg/database/database.gopkg/database/userdb/media_history.gopkg/database/userdb/media_history_property_test.gopkg/database/userdb/media_history_test.gopkg/database/userdb/migrations/20260612000000_create_profiles_table.sqlpkg/database/userdb/profiles.gopkg/database/userdb/profiles_test.gopkg/platforms/platforms.gopkg/service/context.gopkg/service/media_history_tracker.gopkg/service/playtime/limits.gopkg/service/playtime/profile_limits_test.gopkg/service/playtime/provider.gopkg/service/profiles/pin.gopkg/service/profiles/pin_test.gopkg/service/profiles/profiles.gopkg/service/profiles/profiles_test.gopkg/service/profiles/resolver.gopkg/service/profiles/resolver_test.gopkg/service/profiles/switchid.gopkg/service/profiles/switchid_test.gopkg/service/profiles/wordlist.txtpkg/service/queues.gopkg/service/require_profile_test.gopkg/service/scan_behavior_test.gopkg/service/service.gopkg/service/state/active_profile_test.gopkg/service/state/state.gopkg/testing/helpers/db_mocks.gopkg/zapscript/commands.gopkg/zapscript/profile.gopkg/zapscript/profile_test.go
|
|
||
| When no profile is active the device behaves exactly as it did before profiles existed: global playtime limits apply and history is unattributed. | ||
|
|
||
| ##### Profile object |
There was a problem hiding this comment.
Fix heading level jump in the Profiles section.
Line 2458 jumps from ## to #####, which violates markdown heading increment rules and hurts generated outline/navigation.
Suggested diff
-##### Profile object
+### Profile object📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ##### Profile object | |
| ### Profile object |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 2458-2458: Heading levels should only increment by one level at a time
Expected: h3; Actual: h5
(MD001, heading-increment)
🤖 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 `@docs/api/methods.md` at line 2458, The "Profile object" heading is using too
many hashes (shown as "##### Profile object") and jumps from the previous "##"
level; update that heading to the correct level to follow the surrounding
section hierarchy (e.g., change "##### Profile object" to "### Profile object"
or the appropriate level used by the nearby "Profiles" section) so the markdown
outline/navigation is consistent; locate the literal "Profile object" heading in
docs/api/methods.md and adjust the hash count to match the surrounding headings.
Source: Linters/SAST tools
Comment on lines
+420
to
+423
| | Key | Type | Required | Description | | ||
| | :------ | :----- | :------- | :------------------------------------------------------------------ | | ||
| | profile | object | Yes | The new active profile, or null when the device deactivated. | | ||
|
|
There was a problem hiding this comment.
Document profile as nullable in the parameter type.
Line 420 currently lists profile as object, but the description says it may be null when deactivated. Please make the type explicit (object | null) to keep the contract unambiguous.
Suggested diff
-| profile | object | Yes | The new active profile, or null when the device deactivated. |
+| profile | object \| null | Yes | The new active profile, or null when the device deactivated. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | Key | Type | Required | Description | | |
| | :------ | :----- | :------- | :------------------------------------------------------------------ | | |
| | profile | object | Yes | The new active profile, or null when the device deactivated. | | |
| | Key | Type | Required | Description | | |
| | :------ | :----- | :------- | :------------------------------------------------------------------ | | |
| | profile | object \| null | Yes | The new active profile, or null when the device deactivated. | | |
🤖 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 `@docs/api/notifications.md` around lines 420 - 423, The docs declare the
notifications parameter "profile" with type "object" but its description says it
can be null; update the parameter type to explicitly show it is nullable (use
"object | null") so the contract is unambiguous—edit the table row for the
"profile" field in the notifications API docs and change the Type column from
"object" to "object | null" while keeping the existing description.
Comment on lines
+183
to
+190
| // SwitchProfileParams switches the device's active profile. Exactly one of | ||
| // ProfileID or SwitchID selects the target; both omitted (or null) means | ||
| // deactivate. PIN is required when the target profile has one set. | ||
| type SwitchProfileParams struct { | ||
| ProfileID *string `json:"profileId"` | ||
| SwitchID *string `json:"switchId"` | ||
| PIN *string `json:"pin"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the profiles.switch handler validates mutual exclusivity of ProfileID and SwitchID.
# Search for the switch handler implementation
rg -n -A 20 'MethodProfilesSwitch|profiles\.switch' --type=go -g '!*_test.go'Repository: ZaparooProject/zaparoo-core
Length of output: 2791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the handler implementation
rg -n --type=go 'HandleProfilesSwitch' .
# 2) Inspect nearby code for validation (look for ProfileID/SwitchID checks)
rg -n --type=go -S 'ProfileID|SwitchID|profileId|switchId|SwitchProfileParams' pkg/api
# 3) If found, print a focused excerpt around the handler function
# (Use rg again to find the file path and then show the surrounding lines)
file="$(rg -l --type=go 'func\s+HandleProfilesSwitch' . || true)"
echo "handler_file=$file"
if [ -n "$file" ]; then
rg -n --type=go -A 200 'func\s+HandleProfilesSwitch' "$file"
fiRepository: ZaparooProject/zaparoo-core
Length of output: 6274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show switch-related tests around HandleProfilesSwitch
rg -n 'TestHandleProfilesSwitch' pkg/api/methods/profiles_test.go
echo "----"
sed -n '1,280p' pkg/api/methods/profiles_test.goRepository: ZaparooProject/zaparoo-core
Length of output: 7982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the params struct definition
sed -n '140,220p' pkg/api/models/params.go
# Inspect validation helper implementation (where ValidateAndUnmarshal lives)
rg -n --type=go 'func\s+ValidateAndUnmarshal' -S pkg | headRepository: ZaparooProject/zaparoo-core
Length of output: 3642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show validation helper implementation
sed -n '1,220p' pkg/api/validation/validator.goRepository: ZaparooProject/zaparoo-core
Length of output: 6513
Enforce ProfileID/SwitchID mutual exclusivity in profiles.switch handler
SwitchProfileParams docs say “Exactly one of ProfileID or SwitchID selects the target”, but HandleProfilesSwitch simply prioritizes profileId when both are non-nil/non-empty (it takes the ProfileID branch first) and does not error on conflicting inputs. Add validation to reject requests where both profileId and switchId are provided (and cover with a test).
🤖 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/api/models/params.go` around lines 183 - 190, The handler currently
accepts both ProfileID and SwitchID and silently prefers ProfileID; update the
HandleProfilesSwitch request validation to enforce mutual exclusivity per
SwitchProfileParams: if both params are non-nil/non-empty return a 400 error
(with a clear message like "provide exactly one of profileId or switchId")
instead of proceeding, and add a unit/integration test that posts a payload
containing both profileId and switchId and asserts the handler responds with the
expected bad-request error; touch the SwitchProfileParams handling branch in
HandleProfilesSwitch to perform this check early before any branch-specific
logic.
Comment on lines
+61
to
+64
| key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to derive PIN hash: %w", err) | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect pbkdf2.Key function call.
The call to pbkdf2.Key has multiple issues:
- Wrong parameter order: The signature is
func Key(password, salt []byte, iter, keyLen int, h func() hash.Hash) []byte, but the code passes the hash function first. - Wrong type: The
pinparameter is astringbut must be[]byte. - No error return:
pbkdf2.Keyreturns only[]byte, not([]byte, error).
🔧 Proposed fix
- key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen)
- if err != nil {
- return "", fmt.Errorf("failed to derive PIN hash: %w", err)
- }
+ key := pbkdf2.Key([]byte(pin), salt, pinIterations, pinKeyLen, sha256.New)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to derive PIN hash: %w", err) | |
| } | |
| key := pbkdf2.Key([]byte(pin), salt, pinIterations, pinKeyLen, sha256.New) |
🤖 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/service/profiles/pin.go` around lines 61 - 64, The pbkdf2.Key call is
incorrect: use the signature Key(password []byte, salt []byte, iter int, keyLen
int, h func() hash.Hash) []byte; convert the pin string to []byte and pass
parameters in that order (password, salt, pinIterations, pinKeyLen, sha256.New),
capture only the returned []byte (no err) e.g. key := pbkdf2.Key([]byte(pin),
salt, pinIterations, pinKeyLen, sha256.New), then proceed with whatever
encoding/return the derived key instead of checking a non-existent error; update
any references in the same function (e.g., where key, err were used) to reflect
the single returned value.
Comment on lines
+94
to
+97
| key, err := pbkdf2.Key(sha256.New, pin, salt, iterations, len(expected)) | ||
| if err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect pbkdf2.Key function call (same issue as HashPIN).
The call to pbkdf2.Key has the same issues as in HashPIN:
- Wrong parameter order: Hash function should be the last parameter.
- Wrong type:
pinis astringbut must be[]byte. - No error return:
pbkdf2.Keyreturns only[]byte.
🔧 Proposed fix
- key, err := pbkdf2.Key(sha256.New, pin, salt, iterations, len(expected))
- if err != nil {
- return false
- }
+ key := pbkdf2.Key([]byte(pin), salt, iterations, len(expected), sha256.New)🤖 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/service/profiles/pin.go` around lines 94 - 97, The pbkdf2.Key call in
pkg/service/profiles/pin.go is using the wrong parameter order and types and
incorrectly expects an error; update the call to use pbkdf2.Key with password as
[]byte(pin), salt as the []byte salt, iterations as the iter int, key length as
len(expected), and sha256.New as the final parameter (pbkdf2.Key returns only
[]byte), then remove the err handling around that call and adjust subsequent
logic that compares the resulting key (e.g., where `expected` is used) to use
the returned []byte key.
Comment on lines
+311
to
+346
| func (s *Service) checkPIN(p *database.Profile, pin string) error { | ||
| if p.PINHash == "" { | ||
| return nil | ||
| } | ||
| if pin == "" { | ||
| return ErrPINRequired | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| now := s.now() | ||
| attempts := s.pinAttempts[p.ProfileID] | ||
| recent := attempts[:0] | ||
| for _, at := range attempts { | ||
| if now.Sub(at) < pinAttemptWindow { | ||
| recent = append(recent, at) | ||
| } | ||
| } | ||
| if len(recent) >= pinAttemptLimit { | ||
| s.pinAttempts[p.ProfileID] = recent | ||
| s.mu.Unlock() | ||
| return ErrPINRateLimited | ||
| } | ||
| s.mu.Unlock() | ||
|
|
||
| if !VerifyPIN(pin, p.PINHash) { | ||
| s.mu.Lock() | ||
| s.pinAttempts[p.ProfileID] = append(recent, now) | ||
| s.mu.Unlock() | ||
| return ErrPINIncorrect | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| delete(s.pinAttempts, p.ProfileID) | ||
| s.mu.Unlock() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Race condition in PIN rate limiting allows bypass.
The checkPIN method has a TOCTOU (time-of-check to time-of-use) race between lines 333 and 336. The recent slice is computed inside the first lock (lines 322–327), then the lock is released to call VerifyPIN (line 335), then reacquired (line 336) and the stale recent is appended to (line 337).
Race scenario:
- Goroutine A: locks, reads
attempts = [], filters torecent = [], unlocks, starts VerifyPIN (slow) - Goroutine B: locks, reads
attempts = [], filters torecent = [], unlocks, calls VerifyPIN (wrong PIN), locks, appends to map:attempts = [t1], unlocks - Goroutine A: finishes VerifyPIN (wrong PIN), locks, sets
attempts = append([], t2) = [t2], unlocks
Result: The t1 attempt is lost. Future checks see only [t2] instead of [t1, t2], allowing extra PIN attempts beyond the limit.
🔒 Proposed fix: re-filter attempts after acquiring the second lock
if !VerifyPIN(pin, p.PINHash) {
s.mu.Lock()
- s.pinAttempts[p.ProfileID] = append(recent, now)
+ now := s.now()
+ attempts := s.pinAttempts[p.ProfileID]
+ recent := attempts[:0]
+ for _, at := range attempts {
+ if now.Sub(at) < pinAttemptWindow {
+ recent = append(recent, at)
+ }
+ }
+ s.pinAttempts[p.ProfileID] = append(recent, now)
s.mu.Unlock()
return ErrPINIncorrect
}🤖 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/service/profiles/profiles.go` around lines 311 - 346, The checkPIN
function suffers a TOCTOU race: it computes recent from s.pinAttempts under
s.mu, releases the lock to call VerifyPIN, then later reuses the stale recent
when appending, causing lost attempts; fix by re-loading and re-filtering the
current attempts after reacquiring the lock (i.e., after VerifyPIN and before
appending) or by keeping the lock across the VerifyPIN call if acceptable;
specifically, in checkPIN re-acquire s.mu and recompute attempts :=
s.pinAttempts[p.ProfileID] and recent = filtered attempts using pinAttemptWindow
before doing s.pinAttempts[p.ProfileID] = append(recent, now), using the same
pinAttemptLimit/pinAttemptWindow variables to enforce rate limiting correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Summary by CodeRabbit