Provide app version validation#8
Conversation
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
Solid resolution of #6. The design choices are right: tag charset validation applied both at wiring time and re-applied at execution for direct task configuration, lexical containment checks on all five configurable paths (with the symlink limitation honestly documented), and the latest/versions/<tag> cache split so a tag literally named latest cannot alias the rolling cache — with a functional test proving both caches stay separate. Traversal coverage includes the URL-encoded form (a%2f) and asserts no files escape. This also retires the earlier v-prefix-assumption finding, since tags are now used verbatim end to end. Two non-blocking comments below.
| ```kotlin | ||
| embedCode { | ||
| version.set("1.2.4") | ||
| version.set("v1.2.4") |
There was a problem hiding this comment.
The verbatim-tag semantics are a behavioral break for existing configurations: version.set("1.2.4") used to be auto-prefixed to v1.2.4, and after this change it requests a tag literally named 1.2.4, which fails at download with a message that gives no hint about the removed convention. The README documents the new behavior well, but nothing helps someone upgrading.
Cheap mitigations, either or both:
- a one-line migration note here ("previous plugin versions prefixed
vautomatically; set the exact tag now"); - when a pinned download fails and the tag does not start with
v, append a hint to the error: "a release tagv<tag>may exist — previous plugin versions added this prefix automatically."
Related one-liner: the pinned cache moved from build/embed-code/<version> to build/embed-code/versions/<tag>, so caches from earlier plugin versions are orphaned (self-heals via one re-download; stale directories linger until a clean). Not worth documenting for end users, but worth knowing it's expected.
|
|
||
| import org.gradle.api.InvalidUserDataException | ||
|
|
||
| private val validReleaseTag = Regex("[A-Za-z0-9][A-Za-z0-9._+~-]*") |
There was a problem hiding this comment.
Two edge notes on the accepted charset — both fine to leave as-is, but worth a conscious decision:
- Git allows hierarchical tag names (
releases/1.2), and GitHub can publish releases under them; such tags are now impossible to install through this plugin. That's a reasonable fail-closed tradeoff (a/in a cache-directory segment is exactly what Validate configured versions to keep installation paths inside the build directory #6 is about), but consider stating the accepted charset in theversionproperty KDoc / README so a user with a slashed tag reads it as a documented limitation rather than a bug. - A tag equal to a Windows reserved device name (
CON,NUL,AUX,COM1…) passes the regex but produces an unusable cache directory on Windows. Vanishingly unlikely for real releases — mentioning only so the omission is deliberate.
Oleg-Melnik
left a comment
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with minor comments.
2 participants
This PR secures app version and binaries storage paths by adding explicit validation.
Resolves: