Eliaaazzz commented on code in PR #39363:
URL: https://github.com/apache/beam/pull/39363#discussion_r3622083711
##########
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java:
##########
@@ -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("[<>:\"/\\\\|?*]", "_");
Review Comment:
`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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
