12 ms·

> Someone asking you to write a test for new code per the response: "I'm not sure what kind of test would you like me to write for this change, as it's simply

by Chris2048 6mo ago

> Someone asking you to write a test for new code per the response: "I'm not sure what kind of test would you like me to write for this change, as it's simply adding 4 quotes"

shimman 6mo ago
That totally justifies the very normal extortion like blog post in response.
kstrauser 6mo ago
Maybe one showing that the change doesn't make it worse. Here's the code change: - <a class="item muted sidebar-item-link" href=${$(this).data('href')}> + <a class="item muted sidebar-item-link" href="${$(this).data('href')}"> I know zero about this code path, but suppose it's expected that `${$(this).data('href')}` is already a properly quoted value, like `"https://example.com https://example.com"`. Then the first line expands to: <a class="item muted sidebar-item-link" href="https://example.com"> and the second expands to: <a class="item muted sidebar-item-link" href=""https://example.com""> which would have all kinds of room for mischief. Or suppose the template engine auto-quotes values that it injects, so the quotes aren't necessary at all, which is a pretty common approach. The point is that you don't randomly want to throw quotes into HTML or single quotes into SQL just for giggles. You have to write tests demonstrating that the existing common use cases still work after the change, even if it's simply adding 4 quotes.
- MajesticHobo2 6mo ago
  I'd say also add a test that shows the HTML injection (which spurred the PR) isn't possible. Given an attacker-controlled URL of: foo onclick the following shouldn't render: <a class="item muted sidebar-item-link" href=foo onclick> The following should: <a class="item muted sidebar-item-link" href="foo onclick">
  - kstrauser 6mo ago
    Oh, for sure! That'd end the conversation: "your change breaks the existing tests. Fix that and we'll re-consider."
- pabs3 6mo ago
  I wonder why they didn't change it to use DOM APIs instead. Related comment: https://news.ycombinator.com/item?id=47945472 https://news.ycombinator.com/item?id=47945472
  - kstrauser 6mo ago
    Not sure. Are those APIs widely supported now? This is far outside my expertise and I don't know the current state of the art.
    - pabs3 5mo ago
      Yes, document.createElement is widely supported.

> Someone asking you to write a test for new code per the response: "I'm not sure what kind of test would you like me to write for this change, as it's simply adding 4 quotes"

shimman 6mo ago

That totally justifies the very normal extortion like blog post in response.

kstrauser 6mo ago

Maybe one showing that the change doesn't make it worse. Here's the code change: - <a class="item muted sidebar-item-link" href=${$(this).data('href')}> + <a class="item muted sidebar-item-link" href="${$(this).data('href')}"> I know zero about this code path, but suppose it's expected that `${$(this).data('href')}` is already a properly quoted value, like `"https://example.com https://example.com"`. Then the first line expands to: <a class="item muted sidebar-item-link" href="https://example.com"> and the second expands to: <a class="item muted sidebar-item-link" href=""https://example.com""> which would have all kinds of room for mischief. Or suppose the template engine auto-quotes values that it injects, so the quotes aren't necessary at all, which is a pretty common approach. The point is that you don't randomly want to throw quotes into HTML or single quotes into SQL just for giggles. You have to write tests demonstrating that the existing common use cases still work after the change, even if it's simply adding 4 quotes.

MajesticHobo2 6mo ago
I'd say also add a test that shows the HTML injection (which spurred the PR) isn't possible. Given an attacker-controlled URL of: foo onclick the following shouldn't render: <a class="item muted sidebar-item-link" href=foo onclick> The following should: <a class="item muted sidebar-item-link" href="foo onclick">
- kstrauser 6mo ago
  Oh, for sure! That'd end the conversation: "your change breaks the existing tests. Fix that and we'll re-consider."
pabs3 6mo ago
I wonder why they didn't change it to use DOM APIs instead. Related comment: https://news.ycombinator.com/item?id=47945472 https://news.ycombinator.com/item?id=47945472
- kstrauser 6mo ago
  Not sure. Are those APIs widely supported now? This is far outside my expertise and I don't know the current state of the art.
  - pabs3 5mo ago
    Yes, document.createElement is widely supported.