Hacker News Reader

Comment by shimman | Hacker News Reader

Seriously, this author comes across as an absolute sore loser if this is the PR they are referring too:

https://codeberg.org/forgejo/forgejo/pulls/12283

Someone asking you to write a test for new code and then making this blog in response is just so pathetic.

While I agree with you that this blog post (and the "carrot disclosure" described in it) is ill-considered, the pull request is not really "new code", it adds quotes to HTML attributes that are missing them. I think it's entirely reasonable for a contributor to assume that a new test case would not be needed for this small change, and that the maintainer's response ("So a simple question: is this code covered under a test? If not, you will have to add one.") is more abrasive than necessary.

a test is probably not the right thing for this, but adding a linting rule so that quoting is enforced everywhere is probably the right way to go

> 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"

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"`. 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.

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">

Oh, for sure! That'd end the conversation: "your change breaks the existing tests. Fix that and we'll re-consider."

I wonder why they didn't change it to use DOM APIs instead. Related comment:

https://news.ycombinator.com/item?id=47945472

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.

Yes, document.createElement is widely supported.

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

To hell with writing a test for you. That’s what you say to someone who gets paid by you. If the project doesn’t want the fix. That’s their issue, not the reporter’s.

Look at the big picture. The maintainers likely deal with many low-quality bug reports and PRs coming in, especially from AI, and the incentive to spam these is not going away anytime soon. How should they best allocate their limited attention?

One way is for the PR maker to signal their own attention to detail/effort/commitment by jumping through the (quite reasonable) hoop of writing a test.

Is this extra effort? Yes. But if your motivation in opening the PR in the first place is genuinely to improve the world, then do the slightly harder thing that actually improves the world given the constraints on maintainer attention it operates under, not the thing that is slightly easier for you but leaves your contribution indistinguishable from the sea of slop out there.

Reply on news.ycombinator.com