Page MenuHomePhabricator

Don't allow raw pipes in inline Implementations and Testers when sending over the API, as they get munged
Closed, ResolvedPublicBUG REPORT

Assigned To
Authored By
GrounderUK
Feb 21 2024, 11:13 AM
Referenced Files
F42505493: IMG_0887.png
Mar 9 2024, 7:54 PM
F42047557: IMG_0875.png
Feb 22 2024, 7:17 PM
F42019273: IMG_0869.png
Feb 21 2024, 11:13 AM
F42019145: IMG_0874.png
Feb 21 2024, 11:13 AM
F42019133: IMG_0873.png
Feb 21 2024, 11:13 AM
F42019112: IMG_0872.png
Feb 21 2024, 11:13 AM
F42019087: IMG_0871.png
Feb 21 2024, 11:13 AM

Description

When transmitting values up to the tester API, we distinguish multiple values with pipes. Unfortunately, this means that any Implementation or Tester that uses a pipe character anywhere within its body (e.g. in a comment, but critically in
bitwise operations) gets munged half-way through, triggering an invalid-object error.


Steps to replicate the issue (include links if applicable):

  • Go to any Implementation with connected tests in Python or JavaScript. (For example, https://www.wikifunctions.org/view/en/Z13091)
  • Click “Edit source”.
  • Enter a pipe character [“|” – U+007C] anywhere in the code or within a comment.
  • Click the refresh icon in the Tests box.

What happens?:

  • All Tests show as failed.
  • No Test has details of the execution or failure, when “Details” is clicked.
  • Only the Implementation label and the Test label appear in the pop-up.

Trying the same inputs in “Try this implementation” is successful, however.

What should have happened instead?:
Tests should still have passed or, if they fail, details of the execution and error should be viewable on clicking “Details”.

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

The same problem can occur in a composition that contains a literal string (like: https://www.wikifunctions.org/wiki/Z13264).

Insert a “|” within the Z6 value (labelled “suffix” in the example provided) and click the refresh icon in the Tests box. All tests fail without details.

After removing the “|”, Tests will pass or fail as expected after clicking the refresh icon.

IMG_0871.png (2×960 px, 324 KB)
Initial view with successful tests at 22:16.
IMG_0872.png (2×960 px, 320 KB)
“Edit source” with no changes at 22:17.
IMG_0873.png (2×960 px, 290 KB)
No execution details in the pop-up
IMG_0874.png (2×960 px, 402 KB)
Subsequent view at 22:18, after cancelling the edit.
IMG_0869.png (2×960 px, 249 KB)
A previous example showing a successful result for the inputs to a “failed” Test.

The pipe character also breaks a Z20/Test if it is used within the value of a Z6/String while editing the Test.

Event Timeline

GrounderUK renamed this task from All tests fail without details on “Edit source” (though valid) and never pass on refresh to All tests fail without details on “Edit source” (though valid) and never pass on refresh if a pipe character [U+007C] is used.Feb 21 2024, 11:21 AM

This sounds like a horrendous problem, and we've had issues in the past, but I can't replicate – if I edit https://www.wikifunctions.org/wiki/Z13091?action=edit to add a # | comment, or a a = 2|3 line, either way the function appears to work? Can you give a specific case that fails for you? (Or does it come and go?)

This sounds like a horrendous problem, and we've had issues in the past, but I can't replicate – if I edit https://www.wikifunctions.org/wiki/Z13091?action=edit to add a # | comment, or a a = 2|3 line, either way the function appears to work? Can you give a specific case that fails for you? (Or does it come and go?)

That link currently exhibits the problem. I don’t actually see the pipe in there but it has always failed whenever I have entered that character.

IMG_0875.png (2×960 px, 471 KB)

Created https://www.wikifunctions.org/view/en/Z13371

For me, this test containing a pipe is currently “Passed” but exhibits the bug as soon as it is edited.

I have saved Z13091 with a pipe in a comment. To be clear, the implementation always works; it’s just that all the tests show as Failed (erroneously and with no details and only during the edit). If I introduce the pipe during an edit, the tests will not fail until I click the refresh icon in the Tests box. Similarly, they eventually pass (on refresh) if I remove the pipe character.

This particular implementation often has all tests fail for an unrelated reason (resource-related?). However, when they all fail because of the pipe, their repeated failure on refresh is more or less instantaneous, so I guess they don’t even begin re-evaluation when the pipe is present.

This sounds like a horrendous problem…

That link currently exhibits the problem. I don’t actually see the pipe in there but it has always failed whenever I have entered that character.

Thinking about it, I followed your edit link from the email notification, which opened the page in a browser session where I was not logged in… So it appears that the pipe might make it look as though I am not a logged in user (in the case where I am). In any event, I have never seen the empty(ish) details pop-up before, so I was assuming it was part of the problem, as opposed to normal (?) behaviour in the context of a user who is not logged in. There may be a clue there.

OK, so this is (wrongly) triggering a wikilambda-performtest-error-invalidimplementation error, and the UX is failing to handle it. I've filed the second part as a different task, T358295.

Jdforrester-WMF renamed this task from All tests fail without details on “Edit source” (though valid) and never pass on refresh if a pipe character [U+007C] is used to Don't allow raw pipes in inline Implementations and Testers when sending over the API, as they get munged.Feb 22 2024, 10:25 PM
Jdforrester-WMF updated the task description. (Show Details)

Change 1005850 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/extensions/WikiLambda@master] store: When transmitting inline code, encode pipes so they make it

https://gerrit.wikimedia.org/r/1005850

Jdforrester-WMF changed the task status from Open to In Progress.Feb 22 2024, 10:26 PM
Jdforrester-WMF claimed this task.
Jdforrester-WMF triaged this task as High priority.
Jdforrester-WMF moved this task from To Triage to In Progress on the Abstract Wikipedia team board.

Change 1005850 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] store: When transmitting inline code, encode pipes so they make it

https://gerrit.wikimedia.org/r/1005850

That seems to have fixed the problem for a single pipe but not for more than one (separate or together). In https://www.wikifunctions.org/view/en/Z13371, there is a pipe in both the “English word” string (Z13254K1) and in the expected result (Z866K2), so the bug appears when refreshing test results in edit mode. If either pipe is removed the test fails correctly with details of the mis-matched strings.

Is there a correct way for me to suggest that “Verify in production” partially failed?
@Jdforrester-WMF (please see comments above)

I've also noticed this issue, and I have had to re-write my implementations to not use pipe characters in https://www.wikifunctions.org/view/en/Z12711 and https://www.wikifunctions.org/wiki/Z14212

Is there a correct way for me to suggest that “Verify in production” partially failed?
@Jdforrester-WMF (please see comments above)

It's not marked Resolved, so yes, this has failed verification and I'll move back to Ready for someone to take on.

Jdforrester-WMF changed the task status from In Progress to Open.Jun 20 2024, 5:26 PM

This is because str.replace in JavaScript, by default, only replaces the first occurrence of the search string. In PHP replaces all occurances in a string. Ill fix that!

DSmit-WMF changed the task status from Open to In Progress.Aug 7 2024, 12:20 PM
DSmit-WMF claimed this task.

Change #1060423 had a related patch set uploaded (by Daphne Smit; author: Daphne Smit):

[mediawiki/extensions/WikiLambda@master] testResults.js: Encode all pipes in Implementations and Testers when sending over the API

https://gerrit.wikimedia.org/r/1060423

Change #1060423 merged by jenkins-bot:

[mediawiki/extensions/WikiLambda@master] testResults.js: Encode all pipes in Implementations and Testers when sending over the API

https://gerrit.wikimedia.org/r/1060423