feat(s3-publishing): Add S3 static publishing support for vanity URLs#35808
feat(s3-publishing): Add S3 static publishing support for vanity URLs#35808dsilvam wants to merge 13 commits into
Conversation
|
Claude finished @dsilvam's task in 6m 41s —— View job 🔍 dotCMS Backend Review
[🟠 High]
config.put(CURRENT_HOST, host);
config.put(CURRENT_LANGUAGE, Long.toString(language.getId()));
final String bucketName = getBucketName(config);
// ...no restore on exit💡 Capture [🟠 High]
publishAlias(context, alias, file); // S3 PUT
deleteObsoleteAliases(context, persistedAliases, alias); // S3 DELETE
repository.replaceMappingsByVanityUrlId(...); // if this throws, DB is stale💡 Persist the new mapping immediately after the successful S3 push (and before issuing obsolete-clone deletes), or add a compensation path mirroring [🟡 Medium]
private static final String DELETE_BY_VANITY_URL_ID =
"DELETE FROM static_s3_vanity_mapping WHERE endpoint_id = ? AND language_id = ? AND vanity_url_id = ?";💡 Add [🟡 Medium]
canonical_path_hash varchar not null,
vanity_path_hash varchar not null,
primary key (endpoint_id, host_id, language_id, canonical_path_hash, vanity_path_hash)💡 Use [🟡 Medium]
import com.dotcms.business.WrapInTransaction;💡 Delete the import. [🟡 Medium]
private String storageLocation(final S3VanityAlias alias) {
return String.join("|", nullSafe(alias.bucketName), nullSafe(alias.bucketPrefix), alias.vanityPath);
}💡 Either wrap with [🟡 Medium]
if (value instanceof Number) {
return ((Number) value).longValue();
}
return Long.parseLong(String.valueOf(value));💡 Drop the fallback and throw an explicit Next steps
|
|
Semgrep found 16
The method identified is susceptible to injection. The input should be validated and properly If this is a critical or high severity finding, please also link this issue in the #security channel in Slack. |
🔍 dotCMS Backend Review[🔴 Critical]
private static final String UNSUPPORTED_CHARS = "*?[](){}|^$\\+";
...
if (!normalized.startsWith(StringPool.FORWARD_SLASH) || containsUnsupportedChars(normalized)) {
return Optional.empty();
}
return Optional.of(normalized.replaceAll("/{2,}", "/"));💡 URL-decode once, tokenize on [🔴 Critical]
final List<Contentlet> contentlets = contentletAPI.search("+identifier:" + asset.getAsset() + " +live:true",
0, 0, null, APILocator.getUserAPI().getSystemUser(), false);
return contentlets.stream().filter(Contentlet::isVanityUrl).findFirst();💡 Validate the value as a UUID with [🟠 High]
protected String getCompleteFileKey(final String bucketRootPrefix, final String filePath) {
String completeFileKey = filePath;
if (completeFileKey.startsWith(File.separator)) {
completeFileKey = completeFileKey.substring(1);
}
// ...prefix concatenation, no traversal check...
}💡 Defensively reject (throw [🟠 High]
final User systemUser = APILocator.getUserAPI().getSystemUser();
final Optional<String> canonicalPath = aliasSupport.normalizeCanonicalPath(
aliasSupport.getForwardTo(vanityContentlet));
...
target = resolveTarget(context, canonicalPath.get(), systemUser);💡 Resolve/render using the Vanity URL's owner or mod-user ( [🟠 High]
@WrapInTransaction
public void publishAliases(final S3VanityAliasContext context) throws DotDataException {
...
publishAliases(context, aliasesToRefresh, alias -> { }); // S3 PUT loop
deleteAliases(context, aliasesToDelete, deletedNow::add); // S3 DELETE loop
repository.replaceMappings(context.lookup, aliasesToRefresh); // only DB work
}💡 Drop [🟡 Medium]
private static final String DELETE_BY_VANITY_URL_ID =
"DELETE FROM static_s3_vanity_mapping WHERE endpoint_id = ? AND language_id = ? AND vanity_url_id = ?";
private static final String INSERT_ALIAS =
"INSERT INTO static_s3_vanity_mapping (...) VALUES (...)";💡 Switch the INSERT to [🟡 Medium]
[🟡 Medium]
canonical_path_hash varchar not null,
vanity_path_hash varchar not null,
...
primary key (endpoint_id, host_id, language_id, canonical_path_hash, vanity_path_hash)💡 Use [🟡 Medium]
if (value instanceof Number) {
return ((Number) value).longValue();
}
return Long.parseLong(String.valueOf(value));💡 Either trust the JDBC contract and throw on unexpected types, or use Next steps
|
| final User systemUser = APILocator.getUserAPI().getSystemUser(); | ||
| final Optional<String> canonicalPath = aliasSupport.normalizeCanonicalPath( | ||
| aliasSupport.getForwardTo(vanityContentlet)); | ||
| final Optional<S3VanityResolvedTarget> target = resolveTarget(context, canonicalPath.get(), systemUser); |
There was a problem hiding this comment.
canonicalPath.get() without isPresent might result in a NPE
|
|
||
| private static final String CREATED_BUCKETS = "createdBuckets"; | ||
| private static final Lazy<Boolean> STATIC_PUSH_S3_VANITY_ALIAS_ENABLED = | ||
| Lazy.of(() -> Config.getBooleanProperty("STATIC_PUSH_S3_VANITY_ALIAS_ENABLED", false)); |
There was a problem hiding this comment.
This does not allow changes on the fly.
If you cache in a lazy wrapper, you will have to restart to see it take effect
| Logger.info(this, "Sleeping before next push try, seconds: " + secondsToSleep); | ||
| Thread.sleep(secondsToSleep * 1000); | ||
| } catch (InterruptedException ie) { | ||
| Logger.error(this, "Can't Sleep before retry file: " + file.getAbsolutePath()); |
There was a problem hiding this comment.
we're missing a Thread.currentThread().interrupt(); here
| * @param context publishing context | ||
| * @param aliases aliases to remove | ||
| */ | ||
| private void rollbackAliases(final S3VanityAliasContext context, final List<S3VanityAlias> aliases) { |
There was a problem hiding this comment.
This is never called.
its dead code
| } | ||
|
|
||
| /** | ||
| * Indica se il file viene gestito dal publisher S3. |
There was a problem hiding this comment.
italian - must be English
syedATdot
force-pushed
the
s3-static-push-vanity-url-dsilvam
branch
from
d3c5801 to
1f11202
Compare
- Path traversal: add containsTraversalOrUnsafePath() to S3VanityAliasSupport normalizers — URL-decodes once, then rejects .. / . segments, control chars, and residual % (double-encoded sequences) - Lucene injection: validate asset identifier as UUID before concatenating into ES/Lucene query in AWSS3Publisher - Defense-in-depth: add rejectPathTraversal() guard in getCompleteFileKey() before any path becomes an S3 object key - Connection pool: remove @WrapInTransaction from all four public service methods in S3VanityAliasService — S3 I/O now runs outside DB transactions; each repository call manages its own short-lived transaction Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… DDL Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Summary
This PR carries the same changes as #35643 (originally authored by @riccardoruocco), opened under a different account so CI checks can run.
Proposed Changes
STATIC_PUSH_S3_VANITY_ALIAS_ENABLED, to enable Vanity URL handling for AWS S3 static publishing. When the flag isfalse, dotCMS behaves exactly as it does today and no Vanity URL alias is generated.true, publishing a Vanity URL to a static S3 endpoint makes dotCMS resolve the target content, identify the live resource, render or copy it, and write the static clone to the S3 path represented by the Vanity URL.s3_vanity_aliastable, which acts as an operational snapshot of the Vanity URL state on S3 and stores the aliases that have actually been materialized.Additional Info
The goal of this change is to make Vanity URLs work on static S3 publishing in the same way they already work at runtime on live dotCMS.
The implementation is intentionally opt-in and does not affect existing installations unless
STATIC_PUSH_S3_VANITY_ALIAS_ENABLED=trueis set.The
s3_vanity_aliastable is used as the source of truth for what was actually written to S3, so publish, republish, unpublish, and delete can all behave consistently without depending only on the current live content state.Closes #35663