Repository navigation
fix: address security audit hardening notes - #27
Conversation
Share grants and comments: strip share-grant digests from every topology response, SSE event and export; require If-Match on whole-topology PUT; keep server-owned comment threads; attribute comments to the session; revoke grants on organization moves; default share expiry 90 days, max 365. Auth: aggregate IPv6 throttle keys per /64; consume Entra flows only with the matching cookie; revoke the previous session on sign-in; bound concurrent Argon2; bind sealed TOTP secrets to their account (GCM AAD, legacy envelopes resealed at startup); reserve the Guest organization name. Availability: directoryMu is now an RWMutex so topology writes in different organizations never serialize; admin bodies are decoded before the lock; 30s deadline on analysis/trace; per-topology photo byte cap; per-organization map cap; linear-memory trace search; set-based VLAN checks; STP link index built once per analysis; SSE events scoped to their organization. Operations: single-instance Postgres advisory lock; orphaned-media sweep at startup; warnings for untrusted X-Forwarded-For and database-stored TOTP key; photo responses revalidate; documentation iframe drops allow-same-origin; escape remaining server-sourced attributes. CI/deploy: images publish only from main and v* tags; release tags must be on main; pinned Node and checkout versions; configurable Strix endpoint; Compose binds 127.0.0.1 by default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
wiredraft/internal/handler/server.go
Lines 444 to 446 in b57f8e7
Enforce the organization cap when moving maps
When a user has create access in the target organization, a whole-topology PUT can move a map by taking this branch, but it never calls organizationHasCapacity. Consequently, an organization already at the 1,000-map limit can receive an arbitrary number of maps moved from other organizations, bypassing the new resource bound.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expiresAt := now.Add(defaultShareLifetime) | ||
| if input.ExpiresAt != nil { | ||
| expiresAt = input.ExpiresAt.UTC() | ||
| } |
There was a problem hiding this comment.
This expiry is applied only to newly created grants. Existing grants from versions where expiration was optional retain ExpiresAt == nil, and getSharedTopology still treats that value as valid indefinitely, so upgrading does not make those anonymous links expire as intended. Migrate legacy grants to a bounded expiry or reject/prune nil-expiry grants on read.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a86d58: shareExpiry treats grants stored without expiresAt as expiring 365 days after createdAt, both for anonymous reads and in the share listing. Covered by TestLegacyShareGrantsWithoutExpiryStopWorking.
| if !s.organizationHasCapacity(w, request, topology.OrganizationID) { | ||
| return | ||
| } | ||
| created, err := s.store.Create(request.Context(), topology) |
There was a problem hiding this comment.
When multiple requests create or duplicate maps concurrently near the limit, each request can observe the same count below 1,000 before any of their Create calls commit. The shared directoryMu.RLock permits exactly this concurrency, so all requests can succeed and exceed the advertised resource cap; enforce the count and insertion in one serialized or database transaction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7a86d58: creates and duplicates now take a dedicated createMu around the capacity count and the insert, so concurrent requests cannot all pass the check.
Strix security scan: no vulnerabilities reportedCommit 7a86d58 · exit code 0 · workflow run Details are in the workflow run log. |
- Treat share grants stored without an expiry (created before expiry was mandatory) as expiring maxShareLifetime after creation. - Serialize the per-organization map-cap check with the insert so concurrent creates or duplicates cannot exceed the cap. - Drop the size arithmetic in the missingVLANs capacity hint (CodeQL go/allocation-size-overflow). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addresses the 42 hardening notes from the run-1 security audit (
~/security-audit-skill/wiredraft/run-1/REPORT.md). See the commit message for the full list.Not fully addressed: #20 (50 MP photo cap kept, because modern phones produce 48 to 50 MP images); #27 (no per-creator share binding, but grants now expire within 365 days and are revoked on organization moves); #31 (
If-Matchis required only for whole-topology PUT, because sub-resource calls from the SPA don't always send a revision); #33 (the STP link index is built once per analysis, while the per-VLAN link scan remains);minify-js.ymlkeepsnode-version: 24because a contract test pins it.Behavior changes for operators: Compose now binds 127.0.0.1 by default; a second WireDraft instance on the same database refuses to start; share links now always expire; comments are attributed to the signed-in account.
Local checks:
go vet,go test ./...andnpm run test:unitpass, and golangci-lint is clean on the changed files.🤖 Generated with Claude Code