Skip to content

Fix artifact staging filenames on Windows #39363

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

Open
AtharvUrunkar wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −1
Open

Fix artifact staging filenames on Windows #39363

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
4 changes: 3 additions & 1 deletion ...on/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,8 +510,10 @@ private String createFilename(String environment, RunnerApi.ArtifactInformation
// all path separators.
List<String> components = Splitter.onPattern("[^A-Za-z-_.]]").splitToList(path);
String base = components.get(components.size() - 1);
String sanitizedEnvironment = environment.replaceAll("[<>:\"/\\\\|?*]", "_");

@gemini-code-assist gemini-code-assist Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To avoid compiling the regular expression pattern on every invocation of createFilename, it is highly recommended to precompile the pattern as a private static final Pattern constant at the class level.

For example:

private static final Pattern INVALID_WINDOWS_CHARS = Pattern.compile("[<>:\"/\\\\|?*]");
Suggested change
String sanitizedEnvironment = environment.replaceAll("[<>:\"/\\\\|?*]", "_");
String sanitizedEnvironment = INVALID_WINDOWS_CHARS.matcher(environment).replaceAll("_");

@Eliaaazzz Eliaaazzz Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

base on the line above isn't sanitized, and I think it can carry the same characters this is fixing.

The splitter pattern has a stray ] outside the character class, so it only splits on an invalid char followed by a literal ], and base ends up being the whole path:

path=C:\Users\me\artifact.jar  ->  base=C:\Users\me\artifact.jar

That branch is taken when roleUrn != STAGING_TO_ARTIFACT_URN and typeUrn == FILE_ARTIFACT_URN, which is the case for Go pipelines (graphx/translate.go:147-152) and for Python pypi_requirements (stager.py:131-137). Linux doesn't notice because LocalFileSystem.create mkdirs the parents, so it just makes nested directories.

Would it make sense to fix the pattern to [^A-Za-z-_.], or to sanitize the composed name instead of just environment? I traced this statically and haven't reproduced it, so worth a second look.

@Eliaaazzz Eliaaazzz Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: consider an allowlist such as [^A-Za-z0-9-_.] rather than a denylist. It would match the shape of the splitter just above and of SdkComponents.java:204, and it also covers ASCII control characters 0x00-0x1F, which are invalid on Windows as well. Probably can't occur in an environment id, so feel free to ignore.

@Abacn Abacn Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand non-ascii unicode file names are acceptible?

@Eliaaazzz Eliaaazzz Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, you're right. Windows accepts Unicode filenames, so the ASCII-only allowlist would unnecessarily replace valid non-ASCII characters and is probably too restrictive for this fix.

The narrower denylist makes more sense here. If covering control characters is worthwhile, they could instead be added explicitly, for example [\x00-\x1F<>:"/\|?*], while preserving Unicode. Thanks for pointing that out.

@AtharvUrunkar AtharvUrunkar Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for tracing this. Good catch on the splitter pattern and base potentially carrying invalid filename characters as well.

I agree that an ASCII-only allowlist would be unnecessarily restrictive for valid Unicode filenames. I'll update the fix to preserve Unicode while sanitizing Windows-invalid characters, and I'll also take another look at the splitter issue so that both the environment-derived and artifact-derived parts of the generated filename are safe.

I'll add/update the regression tests accordingly.

return clip(
String.format("%s-%s-%s", idGenerator.getId(), clip(environment, 25), base), 100);
String.format("%s-%s-%s", idGenerator.getId(), clip(sanitizedEnvironment, 25), base),

@Eliaaazzz Eliaaazzz Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: a trailing . or space is invalid on Windows too, and can still survive here, either from base or from clip(..., 100) landing on one. Low impact, since Windows strips them on both write and read.

100);
}

private String clip(String s, int maxLength) {
Expand Down
21 changes: 21 additions & 0 deletions ...rc/test/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,27 @@ public void testStageArtifacts() throws InterruptedException, ExecutionException
checkArtifacts(contentsList, staged.get("env2"));
}

@Test
public void testStageArtifactsWithInvalidFilenameCharacters()
throws InterruptedException, ExecutionException {
String environment = "0:ref_Environment_default";
List<String> contentsList = ImmutableList.of("artifact-content");

stagingService.registerJob(
"stagingToken",
ImmutableMap.of(
environment,
Lists.transform(contentsList, FakeArtifactRetrievalService::resolvedArtifact)));

ArtifactStagingService.offer(new FakeArtifactRetrievalService(), stagingStub, "stagingToken");

Map<String, List<RunnerApi.ArtifactInformation>> staged =
stagingService.getStagedArtifacts("stagingToken");

assertEquals(1, staged.size());
checkArtifacts(contentsList, staged.get(environment));
Comment on lines +186 to +187

@gemini-code-assist gemini-code-assist Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since : is a valid filename character on Unix-like operating systems (such as Linux and macOS), this test will pass on those platforms even without the sanitization fix. To ensure the test actually verifies the sanitization behavior across all platforms (including CI environments running on Linux), consider asserting that the physical file created in the staging directory contains the sanitized environment string (0_ref_Environment_default) and does not contain the colon (:).

}

private void checkArtifacts(
List<String> expectedContents, List<RunnerApi.ArtifactInformation> staged) {
assertEquals(
Expand Down
Loading