Conversation
AllowedPackages was only checked on object creation flows (CreateObject, CreateAndActivateProgram, CreateClassWithTests). Existing-object mutations via EditSource, WriteProgram, WriteClass, WriteSource, RenameObject, and low-level UpdateSource/DeleteObject/CreateTestInclude/UpdateClassInclude bypassed the package restriction entirely. Added checkObjectPackageSafety (resolves package via SearchObject) to all write paths operating on existing objects. Also hardened i18n writes (WriteMessageClassTexts, WriteDataElementLabels) and added transportable edit checks where missing. UI5 BSP mutation paths remain as a tracked follow-up (needs app-to-package resolution). Reported by Philip Dolker. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of the package safety fix session: bug diagnosis, root cause, functions hardened, test coverage, remaining UI5 gap, and pending actions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consolidates operation-type, package-ownership, and transportable-edit
checks behind a single `checkMutation(ctx, MutationContext)` call in a
new pkg/adt/mutation_gate.go. Every mutator (CRUD, workflows, i18n, UI5)
now passes through this gate with an explicit context describing what
it intends to do, instead of wiring three separate sub-checks by hand.
This closes a whole class of "forgot one sub-check" bugs: the package
enforcement bug fixed in the prior commit happened because EditSource
etc. had an op-type check but no package check. The unified gate makes
that omission impossible — you either call checkMutation (which does
all three) or you don't gate at all (obvious at review time).
Surfaces:
- SurfaceADT: resolves package via SearchObject for existing objects
- SurfaceUI5: no app→package resolver yet, so mutations fail closed
when AllowedPackages is configured (was previously a silent bypass)
Precedence: explicit Package > resolve via ObjectURL > fail closed with
a clear error naming the op.
Behavior change: UI5UploadFile, UI5DeleteFile, and UI5DeleteApp now
block when SAP_ALLOWED_PACKAGES is set. They were previously unguarded.
Users combining AllowedPackages with UI5 writes will see a clear error
directing them to the follow-up (UI5 app→package resolution).
Tests:
- New pkg/adt/mutation_gate_test.go covering op-type block,
explicit-package block, ObjectURL resolution, UI5 fail-closed, UI5
pass-when-no-policy, fail-closed on missing context, and end-to-end
UI5UploadFile/DeleteFile/DeleteApp blocking
- Existing tests still pass (go test ./... clean)
Reviewed by Codex.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Uff, that was fast! Thanks :) |
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
2 participants
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
SAP_ALLOWED_PACKAGESwas only enforced on object creation paths, not on existing-object mutationscheckObjectPackageSafetyto all write paths:EditSource,WriteProgram,WriteClass,WriteSource(update path),RenameObject,UpdateSource,DeleteObject,CreateTestInclude,UpdateClassInclude,WriteMessageClassTexts,WriteDataElementLabelsUI5UploadFile,UI5DeleteFile,UI5DeleteApp) remain as follow-up — needs app-to-package resolutionReported by Philip Dolker.
Test plan
go test ./...)checkObjectPackageSafetynormalization tests passUpdateSource/DeleteObject/CreateTestInclude/WriteMessageClassTextsenforcement tests passSAP_ALLOWED_PACKAGES=$TMPand verifyEditSourceon a non-$TMP object is blocked🤖 Generated with Claude Code