Skip to content
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.

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

fix: externalize links fails with ac scriptlets #2388

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Buuhuu
Copy link
Collaborator

@Buuhuu Buuhuu commented Nov 25, 2022

Q                       A
Fixed Issues? Fixes #2387
Patch: Bug Fix? yes
Minor: New Feature? no
Major: Breaking Change? no
Tests Added + Pass? Yes
Documentation Provided
Any Dependency Changes? no
License Apache License, Version 2.0

With this change the url is masked before it is passed to the Externalizer by the DefaultPathProcessor.

Eventually it makes sense to do this in all cases (sanitize, map, exteranlize). To be discussed.

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #2388 (8cc4998) into main (22b9faa) will decrease coverage by 0.01%.
The diff coverage is 92.50%.

❗ Current head 8cc4998 differs from pull request most recent head 91be3a8. Consider uploading reports for the commit 91be3a8 to get more accurate results

@@             Coverage Diff              @@
##               main    #2388      +/-   ##
============================================
- Coverage     87.26%   87.24%   -0.02%     
- Complexity     2517     2521       +4     
============================================
  Files           220      221       +1     
  Lines          6722     6728       +6     
  Branches       1019     1019              
============================================
+ Hits           5866     5870       +4     
- Misses          339      340       +1     
- Partials        517      518       +1     
Impacted Files Coverage Δ
...components/internal/link/DefaultPathProcessor.java 92.95% <77.77%> (-2.50%) ⬇️
...core/components/internal/link/LinkMaskingUtil.java 96.66% <96.66%> (ø)
...core/components/internal/link/LinkBuilderImpl.java 96.39% <100.00%> (-0.04%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

assertEquals(path, underTest.externalize(path, localContext.request()));

verify(externalizer).publishLink(any(ResourceResolver.class), url.capture());
assertFalse(url.getValue().contains("<%="));
Copy link
Member

Choose a reason for hiding this comment

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

I would feel more confident if we asserted on the expected output and not on simply not having the original fragments. This would also catch situation where we might end up up with multiple masking/encoding.

@sonarcloud
Copy link

sonarcloud bot commented Dec 19, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

} else {
externalPath = externalizer.publishLink(resourceResolver, path);
LOG.warn(message, path, ex.getMessage());

Check failure

Code scanning / CodeQL

Insertion of sensitive information into log files

This [potentially sensitive information](1) is written to a log file.
} catch (Exception ex) {
String message = "Failed to externalize url '{}': {}";
if (LOG.isDebugEnabled()) {
LOG.warn(message, path, ex.getMessage(), ex);

Check failure

Code scanning / CodeQL

Insertion of sensitive information into log files

This [potentially sensitive information](1) is written to a log file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links with Adobe Campaign scriptles are not externalized
2 participants