AMP-31197 AMP Security Review Triage - #4508
Open
brianbrix wants to merge 12 commits into
Open
Conversation
AMP-SEC-001 / 002 / 031: /rest/** bypasses Spring Security, and @ApiMethod.authTypes() defaults to no auth,
Contributor
There was a problem hiding this comment.
Pull request overview
This PR appears to address security-review findings by shifting AMP REST endpoint authorization to a fail-closed model: endpoints without explicit auth rules now require authentication by default, and endpoints intended to be public are explicitly marked as such.
Changes:
- Introduces
AuthRule.PUBLICand updates authorization logic to default toAUTHENTICATEDwhen noauthTypesare declared. - Updates many REST endpoints to explicitly declare
authTypes = AuthRule.PUBLICwhere anonymous access is intended. - Adjusts REST-related security and Swagger authorization rule handling to align with the new
PUBLICconcept.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| amp/src/main/webapp/WEB-INF/applicationContext.xml | Replaces /rest/** security="none" with a Spring Security <http> block (CSRF disabled, stateless session policy, digest entry point configured). |
| amp/src/main/java/org/digijava/kernel/ampapi/swagger/SwaggerAuthorization.java | Adds AuthRule.PUBLIC to the set of rules ignored by Swagger authorization decoration. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/util/ApiMethod.java | Documents the new fail-closed default and instructs using PUBLIC for anonymous access. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/security/AuthRule.java | Adds AuthRule.PUBLIC to explicitly mark endpoints as anonymous. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/security/ActionAuthorizer.java | Implements fail-closed default auth behavior and explicit PUBLIC opt-out logic. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/publicportal/PublicEndpoint.java | Marks several public-portal endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/dashboards/EndPoints.java | Marks several dashboard endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/common/TranslationsEndPoints.java | Marks translation-related endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/common/FiltersEndpoint.java | Marks many filter endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/common/CommonEndpoint.java | Marks FM-related endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/common/AmpConfiguration.java | Marks settings/config endpoints as explicitly PUBLIC. |
| amp/src/main/java/org/digijava/kernel/ampapi/endpoints/calendar/CalendarEndpoint.java | Marks calendar retrieval endpoint as explicitly PUBLIC. |
| amp/src/main/java/org/dgfoundation/amp/onepager/components/fields/AmpComponentField.java | Minor whitespace-only change in exception handling line. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @Path("/finstrument") | ||
| @Produces(MediaType.APPLICATION_JSON + ";charset=utf-8") | ||
| @ApiMethod(ui = false, id = "ftype") | ||
| @ApiMethod(ui = false, id = "ftype", authTypes = AuthRule.PUBLIC) |
AMP-SEC-017 / 054: AMP passwords are stored and compared in plaintext via NoOpPasswordEncoder
AMP-SEC-017 / 054: AMP passwords are stored and compared in plaintext via NoOpPasswordEncoder
AMP-SEC-006 / 077: POST /rest/indicator/indicator-layer/import is unauthenticated and accepts arbitrary Excel
AMP-SEC-004 / 005: Resource read/upload endpoints are unauthenticated
…XML parsing lack XXE protections.
…e from previous commit)
…sted digi.xml config digester digi.xml intentionally uses a DOCTYPE-declared SYSTEM general entity (&commonEntities;) to include digi-common.xml. Disallowing DOCTYPE declarations in DigesterFactory.newDigester broke Tomcat startup (DigiConfigManager/DigiXmlECSLoaderListener). This factory only ever parses trusted, locally-deployed config files, so DOCTYPE and general entities are now allowed there; external parameter entities and external DTD loading stay disabled.
ampTemplate/site-config.xml declares internal-only general entities (&Version;, &releaseDate;, etc.) via a DOCTYPE internal subset. Disallowing DOCTYPE broke ShowLayout for any page rendered through this template. Allow DOCTYPE here (still fully local/internal, no SYSTEM/external entity involved).
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
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.
No description provided.