Repository navigation
fix(services): nil-deref in deploy EmptyBucket, dead org-discovery Errors field, ecr dedup - #1070
Conversation
…, docker tag validation Addresses findings 07-M4, 07-L1, 07-L4, 07-N1, 07-N3 from the code review. - deploy/frontend.go: replace *delErr.Key/*delErr.Message raw dereferences with aws.ToString() in EmptyBucket; S3 Error.Key/Message are *string and can be nil when only Code is set, causing a panic during bucket teardown (07-M4) - accounts/org_discovery.go: remove dead Errors []error field from OrgDiscoveryResult; discovery is all-or-nothing -- the field was never populated and no caller reads it (07-L1) - deploy/ecr.go: refactor LoginToPublicECR to delegate to decodeBase64Token instead of inlining the same decode+split logic; improves error message precision (07-L4) - deploy/docker.go: add validateDockerRef helper with a regexp that requires refs to start with an alphanumeric character, rejecting leading hyphens that docker would interpret as flags; applied to BuildImage/TagImage/PushImage (07-N1) - iacfiles/embed.go: add security note documenting that text/template performs no shell escaping and that the renderer owns validation of user-controlled fields (ContactEmail, AccountName, etc.) before template.Execute (07-N3) Closes #1057
|
Warning Review limit reached
More reviews will be available in 11 minutes and 38 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 Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Fixes five robustness/correctness gaps in the deploy, accounts, and iacfiles packages. Closes #1057.
07-M4 -- nil-deref panic in EmptyBucket (Medium)
internal/deploy/frontend.go:*delErr.Keyand*delErr.Messagewere dereferenced unconditionally inside the S3 per-object error loop. Both fields are*stringand can be nil (e.g. an error with onlyCodeset). Fix:aws.ToString()for both, which the package already imported.Regression test:
TestFrontendService_EmptyBucket_PerObjectErrorNilFieldsexercises this with nil Key/Message and confirms no panic.07-L1 -- Dead Errors field in OrgDiscoveryResult (Low)
internal/accounts/org_discovery.go:OrgDiscoveryResult.Errors []errorwas declared but never populated and no caller read it. Discovery is all-or-nothing. The dead field implied partial-success semantics that did not exist. Removed.07-L4 -- Duplicate base64-decode logic in LoginToPublicECR (Low)
internal/deploy/ecr.go:LoginToPublicECRinlined the samebase64.StdEncoding.DecodeString+SplitN(":", 2)+parts[1]logic already encapsulated bydecodeBase64Token, with a less precise error message. Refactored to calldecodeBase64Tokenand delete the inline copy.07-N1 -- Docker tag/name passed without validation (Nit)
internal/deploy/docker.go: AddedvalidateDockerRefwith^[A-Za-z0-9][A-Za-z0-9._/:@-]*$. First character must be alphanumeric so leading hyphens (which docker interprets as flags) are rejected. Applied toBuildImage,TagImage, andPushImage.07-N3 -- IaC templates interpolate user-controlled fields into shell scripts (Nit)
internal/iacfiles/embed.go: Added a security note to the package doc documenting thattext/templateperforms no shell escaping and that the renderer (handler_federation.go) owns validation of user-controlled fields. AddedTestCLITemplatesShellMetacharsPassThroughto confirm the pass-through behavior and document the renderer's responsibility.Verification
No live Postgres required; all covered by unit tests with mock clients.
Open questions for reviewer
ContactEmail,AccountName,CUDlyAPIURL, andAccountSlugare validated/quoted at the API boundary inhandler_federation.gobeforetemplate.Execute. If not, a follow-up fix is needed there.