Skip to content

[OB4] Removing session data key consent from the cache #915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hasithakn merged 1 commit into from
Mar 13, 2026
Merged

[OB4] Removing session data key consent from the cache #915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...so2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,13 @@ public Response persist(@Context HttpServletRequest request, @Context HttpServle
throws ConsentException, ConsentManagementException, URISyntaxException {

ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey);
Copy link
Contributor

@wso2-engineering wso2-engineering bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Improvement Suggestion No: 1

Suggested change
ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey);
ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey);
if (consentData == null) {
log.warn("No consent data found in cache for sessionDataKey: {}", sessionDataKey);
} else {
log.debug("Retrieved consent data from cache for sessionDataKey: {}", sessionDataKey);
}


/*
Remove consent data from the cache to prevent multiple calls to the persist endpoint.
This ensures the flow is completed and avoids further updates after a single persistence.
*/
ConsentCache.removeFromCache(sessionDataKey);
Copy link
Contributor

@wso2-engineering wso2-engineering bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Improvement Suggestion No: 2

Suggested change
ConsentCache.removeFromCache(sessionDataKey);
ConsentCache.removeFromCache(sessionDataKey);
log.info("Removed consent data from cache for sessionDataKey: {}", sessionDataKey);

Comment on lines 258 to +264
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if the base cache class has an atomic getAndRemove operation that could be used
ast-grep --pattern $'class FinancialServicesBaseCache {
  $$$
}'

# Also check for any existing atomic cache operations in the codebase
rg -n "getAndRemove|removeAndGet|atomicRemove" --type java

Repository: wso2/financial-services-accelerator

Length of output: 61

🏁 Script executed:

# Find the ConsentCache class
fd -type f -name "*ConsentCache*" --type f

Repository: wso2/financial-services-accelerator

Length of output: 249

🏁 Script executed:

# Search for ConsentCache class definition and methods
rg -n "class ConsentCache|public.*get.*Cache|public.*remove.*Cache" --type java -A 3

Repository: wso2/financial-services-accelerator

Length of output: 26398

🏁 Script executed:

# Look at the complete flow in ConsentAuthorizeEndpoint to understand the race window
rg -n "getConsentDataFromCache|removeFromCache|persistConsent" --type java -B 2 -A 2

Repository: wso2/financial-services-accelerator

Length of output: 14580

🏁 Script executed:

# Get the full implementation of FinancialServicesBaseCache to understand synchronization
cat financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/caching/FinancialServicesBaseCache.java | head -200

Repository: wso2/financial-services-accelerator

Length of output: 6279

🏁 Script executed:

# Check if there's any synchronization in the ConsentCache specifically
rg -n "synchronized" financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java

Repository: wso2/financial-services-accelerator

Length of output: 129

🏁 Script executed:

# Look at the complete ConsentAuthorizeEndpoint flow around the cache removal to understand persistence logic
sed -n '250,380p' financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java

Repository: wso2/financial-services-accelerator

Length of output: 7306

🏁 Script executed:

# Check the git history to see if lines 273, 284, 289 were modified in this PR
git log -p --follow -S "consentData.getRedirectURI()" financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java | head -100

Repository: wso2/financial-services-accelerator

Length of output: 61

🏁 Script executed:

# Show the exact context around line 273 and 284 to understand the NPE issue
sed -n '268,295p' financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java

Repository: wso2/financial-services-accelerator

Length of output: 1655

Race condition window remains between retrieval and removal.

The cache removal improves security but two concurrent requests can both retrieve valid consentData before either executes the removal. For stronger guarantees, consider implementing an atomic get-and-remove operation in the cache layer.

If atomic operations aren't feasible, this fix still meaningfully narrows the attack window and is acceptable for merge. The database cleanup in the finally block (lines 367-380) provides an additional layer of protection.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java`
around lines 258 - 264, There is a race between
ConsentCache.getConsentDataFromCache(sessionDataKey) and
ConsentCache.removeFromCache(sessionDataKey) allowing two threads to both read
consentData; change the cache API to provide an atomic get-and-remove operation
(e.g., add and call ConsentCache.getAndRemove(sessionDataKey) or similar) and
update ConsentAuthorizeEndpoint to use that atomic method instead of separate
get and remove calls so a single thread obtains and clears the entry atomically.


URI location;
try {
if (consentData == null) {
Expand Down
9 changes: 9 additions & 0 deletions ...java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,13 @@ public int setModifiedExpiryMinutes() {

return FinancialServicesConfigParser.getInstance().getCommonCacheModifiedExpiryTime();
}

/**
* Remove from the consent cache
* @param cacheKey Cache key
*/
public static void removeFromCache(String cacheKey) {

ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey));
}
Comment on lines +230 to +234
Copy link
Contributor

@wso2-engineering wso2-engineering bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Improvement Suggestion No: 3

Suggested change
*/
public static void removeFromCache(String cacheKey) {
ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey));
}
public static void removeFromCache(String cacheKey) {
log.info("Removing consent from cache for key: " + cacheKey);
ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey));
}

}
Loading