Repository navigation
Support special characters in workflow, step and credential names - #5106
Conversation
|
Based on my review of the PR changes, here is my security review. Security Review ✅
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5106 +/- ##
======================================
Coverage 90.7% 90.7%
======================================
Files 419 421 +2
Lines 20691 20812 +121
======================================
+ Hits 18772 18884 +112
- Misses 1919 1928 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
elias-ba
changed the title
elias-ba
changed the title
elias-ba
changed the title
elias-ba
changed the title
ada8116 to
f313316
Compare
| const fileName = useMemo(() => { | ||
| if (!workflow) return 'workflow.yaml'; | ||
| // Remove special characters, replace spaces with hyphens | ||
| // Names can be any script now, and stripping everything outside ASCII |
There was a problem hiding this comment.
now should not be in the comment here. AI should not be commenting the diff, only the current state of the code.
There was a problem hiding this comment.
Thanks for the catch. Fixed
| // leaves a CJK or Arabic name as the empty string and the download as | ||
| // `.yaml`. Fall back to a usable name when nothing survives. | ||
| const sanitized = workflow.name | ||
| .replace(/[^a-zA-Z0-9-_\s]/g, '') |
There was a problem hiding this comment.
I can't help but wonder if we should be using some kind of library for this, consistently across all platforms.
For the workflow download it's not super important to santize the name correctly. But I do think we should have a single "sanitize" function shared or duplicated across repos and we just call that
There was a problem hiding this comment.
This is only a download filename, so it doesn't need to match anything. Where matching did matter was the step id, and OpenFn/kit#1535 removes that by recording the id instead of deriving it.
|
@elias-ba my biggest initial concern is this Users can now input basically any step name, with emoji and non ASCII characters. The CLI and runtime are mostly agnostic to this (they don't care), EXCEPT that the CLI will convert a step name into a step ID. Basically by making it URL safe with a slugify function. That gives us local yamls like this: Notice special characters are dropped . So if you have I also think we should consider: are we making changes here that would prevent us build semantic URLs like What I think we should do in the short term is:
|
|
Btw the CLI side is looking absolutely fine. Workflow yamls are loaded safely as utf8, display names are respected and used, and step ids are safelt generated. So no work there that I can see. Havne't checked the worker but it should be the same (and easy to test from this branch in QA!) |
There was a problem hiding this comment.
Dude, I won't lie. I barely understand this kinda stuff 😅 . I've read it twice and can't hold it in my head.
So we're going for a single printable line and so for that single quotes with doubled apostrophes I think are the right way to go (i.e. 'O''Brien' over "O'Brien") that way you don't need any backslash processing at all. So stoked we do that. But I think the double quoted fallback can't actually be reached, because the name validator stops that before we get here.
So theres a bit of tension between us writing a generic scalar encoder for yaml, and us making our exporter work properly. For the former we need to be really careful because this is what libaries with big test suites are for (i haven't checked if something like this exists inside any existing libraries). So we have a choice, either make doubly sure we are making this module bulletproof or relax/delete some of the unreachable (because of the code we have before this is called) and add something like:
def encode_value(value) do
if String.match?(value, @control_chars) do
raise ArgumentError, "value reached the YAML encoder unvalidated: #{inspect(value)}"
end
...
endThat way if we add a new field to the export and we forget to validate it, we get a bang vs silently broken yaml in someones repo.
There was a problem hiding this comment.
The double quoted branch is not unreachable, so I would rather not delete it. Ordinary names go through it, a Japanese name or a credential called MailChimp June'24. encode_key sends every key that is not bare-legal there, and that is every name in a non-Latin script and every name with a colon, an apostrophe or an emoji.
On the raise, you are right that the validator stops control characters before this point. I am not sure it earns its place though. A field we forgot to validate still comes out as correct escaped YAML today, so the raise would turn a working export into a crash. Happy to be talked round if you think the loud failure is worth more.
2040146 to
f248647
Compare
Job and workflow names were restricted to ASCII letters, digits, spaces, underscores and hyphens, so teams working in French, Spanish, Arabic or Japanese could not name a step in their own language. Names may now hold any character except a control one. The rule lives in Lightning.Utils.Validators and is mirrored client-side in nameValidation.ts, which counts graphemes with Intl.Segmenter so it never disagrees with Ecto. The YAML export was built by string concatenation with a hand-rolled regex, which could not survive the wider character set. ExportUtils.Scalar now owns quoting, escaping and block scalars, and is byte-identical to the old encoder for everything the old encoder wrote correctly. Two names that hyphenate to the same spec key now raise DuplicateKeyError instead of silently dropping one of the pair. Closes #4577
The pre-flight fails with a plain string naming both colliding entities. Two places in the sync component threw it away. reconnect_github/2 matched it on its catch-all and told the user they lacked access to the GitHub installation, which is false. error_message/1 unwrapped any Tesla.Env by recursing into its body, and Tesla.Middleware.JSON leaves the body a binary for an HTML or plain-text response, so a GitHub 502 surfaced a page of HTML in a flash. Only a decoded body is worth unwrapping. The channel already handled this correctly and now has a test for it, as does the reconnect form. Also corrects the cost note above initiate_sync/2: 0.13ms measures the encode, not the preload and workflow queries that feed it.
JS .trim() and String.trim/1 are different sets in both directions. JS leaves U+0085, which Elixir strips, so the client rejected a name the server would have accepted. JS eats U+FEFF, which Elixir keeps, so a name the API had stored was silently rewritten in the editor and its hyphenated key no longer matched the row it came from.
initiate_sync/2 already declared binary(), but configure_github_repo/2 and reconfigure_github_connection/3 pass that error straight up and both said map(). Dialyzer took them at their word and called the binary clause in the sync component unreachable.
Adds tests for the paths that carry a duplicate-key message to the user: the initiate-sync button and the reconnect form both reach error_message/1, and only the reconnect side was covered. Also covers the validator clauses that guard a non-string value, and the two scalar shapes made of nothing. Validators.control_chars_regex/0 is deleted. Nothing called it; the only reference was a comment in ecto_types.ex, which now points at the module attribute instead.
Two names that hyphenate to one spec key stop the whole export. The fallback controller answers a binary error with a 400 JSON body, and this is the one export path with a person on the other end, so they got a wall of text instead of the two names to rename.
The collision it looked for now fails with a message naming both entities at every path that hits it: GitHub sync, the reconnect form and the project export. A query telling an operator the same thing earlier is a convenience, not a safeguard. The other half looked for control characters in legacy names, which do not break those rows: validate_name fires on a change to the name, so such a row still loads and still saves when another field is edited. It also invented a priv/repo/checks/ directory for one file, where this repo keeps such scripts flat in priv/repo.
The export pre-flight lived in initiate_sync/2, which configure_github_repo/2 reaches only after pushing pull.yml, the workflow files and the API secret. create_github_connection/2 wraps that in a transaction, so a project whose names collide left a modified repo behind and no connection row to show for it. The check now runs before the first push.
kind was "job in Payroll" and the message appended an s, so it read "two job in Payrolls in this project". Callers now pass the plural.
workflow_templates.tags is an unbounded character varying[], not a 255-wide one, so there was no 22001 to guard against. The cap only rejected tags that used to save, with no client-side counterpart, and the test asserted the same false premise.
collection_items.value is varchar(1000000) counted in codepoints, while validate_length counts graphemes. Same gap this branch closes on nine other fields.
The ecto_types comment pointed at Lightning.Utils.Validators, which does not exist; the module is Lightning.Validators. And the binary clause in the sync component claimed a plain string is always ours, when refresh_oauth_token/1 passes GitHub's body straight through.
The branch had grown 960 comment lines against about 1,400 lines of code, and the problem was not individual comments but repetition. The grapheme versus codepoint explanation appeared in substance nineteen times, and the "used to raise 22001 on insert" archaeology twelve times, mostly above calls to validate_name_fits_column and validate_no_null_bytes whose own @doc already says it. Removes 296 lines. What stays is the measured evidence itself, stated once, where it is needed: the parser corpus, the PCRE character class trap, why there is no Intl.Segmenter fallback, and the byte-identity constraint on the encoder. Also converts four JSDoc blocks on module-private bindings to plain comments, merges an orphaned JSDoc in workflow.ts, and replaces two references that pointed at line numbers rather than at the thing they meant.
The provisioner compared the spec's credential name to the stored one byte for byte. The stored side went through validate_name and is NFC; the spec body never touches a changeset. Credential names were ASCII-only before this branch, so widening them made a deploy fail on a name that renders identically to the one the user can see, with an error naming that same credential. Channel names get the name rule too. A channel name is an identity key in the project spec and had no rule at all, so two that differ only by normal form export as two keys nothing can tell apart.
custom_path already has a stricter format rule and a length cap from #5105, so the null-byte and width guards this branch added to it are dead weight and the assertions were checking for a message that no longer surfaces. The invisible-codepoint fixture gains U+0890, U+0891, U+08E2 and U+110CD. Erlang 28 brought PCRE2 with newer Unicode tables, which classify those four as Format where the old build did not. For the same reason the client is no longer stricter than the server on that set, so the test that pinned the four-codepoint skew now pins zero.
Comments that said what the code used to do read as notes on the diff rather than on the code. Rewritten to describe current behaviour.
The es2020 lib this compiles against does not declare Intl.Segmenter, so the module carried its own declarations for it. Adding es2022.intl to the lib list gets the real ones and drops the hand-written pair.
The tooltip was built by concatenating HTML and escaping the parts by hand, which only holds while everyone remembers to call escape_html. The content now sits in a template element that HEEx escapes, and the hook clones that node into tippy, so no caller decides whether a tooltip is markup. Elements that set data-allow-html directly are unchanged.
It reads as a fallback for control characters. It is the ordinary path for every key that is not bare-legal.
f248647 to
05d302a
Compare
|
@elias-ba this is still working great on CLI: I can pull (v1 and v2) and execute steps with emojis and special characters I've still got open concerns about validation in the app and ensuring consistent name encoding. Do we have any time to look into this? |
|
@josephjclark you were right about the step ids colliding. If you have two steps called |
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Looks good to me.
I've spun out my validation edge case into #5156 - but I don't think it's worth the time to fix now.
Description
Step names were ASCII-only, so a team working in French, Spanish, Arabic or Japanese could not name a step the way they say it. Names may now hold anything except a control character.
Stacked on #5109, which moves us to Erlang 28. That matters here: Erlang 27 normalises names incorrectly, and this is the PR that starts normalising them.
That broke the YAML export, which concatenated strings against a hand-rolled regex.
ExportUtils.Scalarreplaces it under one constraint: anything the old encoder wrote correctly comes out byte-identical, because customers keep their specs in git and a quoting change is a diff in every synced repo. Also fixes #2808 and #2966.An unrelated XSS fix rides along because it is the same code path.
wrapper_tooltiphard-codeddata-allow-html="true", so a workflow named with an<img onerror>fired for anyone viewing the project list.Closes #4577
Validation steps
Vérifier l'étator患者確認. It saves and survives a reload.My FlowandMy-Flowin one project, then try sync, the reconnect form and "Export project". Each fails naming both.<img src=x onerror=alert(1)>and open the project list. You should see that text sitting in the page as a name, and no alert box. Before this PR the browser ran it, for anyone who opened the page rather than just whoever typed it.Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)