When 'perfect' code fails | Hacker News Reader

When 'perfect' code fails

marma.dev

63 pointsvinhnx53 comments

I think you're looking for `import 'server-only'` and not "use server";. Use server exposes functions as endpoints to the client. I.e. they are simply obfuscated api endpoints without boilerplate. Their main use is for mutations such as a form submission from the client.

Since pages are, by default, SSR, you don't need to have the server call out to itself to run an endpoint to check permissions. Instead, the server should just run the function.

I'm pretty sure Next does some behind the scenes black magic optimizations and doesn't actually make an API request over the wire, but it's still running through some layer of abstractions (causing it to be async) to run the function when instead it could simply be a synchronous function if implemented properly.

These abstractions make sense if you know how to use them properly, but I honestly blame Nextjs for the whole server action confusion. I remember when they first came out and seeing how almost every single question on /r/nextjs was about being confused about server actions. All these footguns and confusion to avoid some boilerplate... I'm not sure if they've improved it since, but I've moved to Svelte and haven't looked back.

Yes you are right and after our learning we changed the code to not use `use server` anymore for this kind of operations.

The documentation and tooling definitely got better and I don't think that such a situation is possible with the latest versions.

I just hope that some people who are still running the specific Next.js version won't fall into this as we did.

SSR = Server-side rendered

This is why you should:

- Write functional tests, not unit tests

- Not use compilers or other systems that do a lot of black magic (like changing the type signature of your functions (!))

I almost never write single function unit tests. There's usually some subset of the codebase that's self contained that makes sense to be the "unit" you're testing, but it's almost always a handful of functions with a clear entry point.

My general rule is to never mock or remove anything that has no side effects from the execution path of a test, even if it's some utility function that's already tested in isolation in its own test suite - trying to isolate every little bit of behaviour to be tested separately is just a bunch of extra work for questionable benefit.

I still call these tests "unit tests", and I think a lot of people do also. But there are the dogmatic people to whom only a test covering a single function with all dependencies mocked out is a true unit test.

This made me laugh because I can't name a single compiler that _doesn't_ do black magic.

But I concede that a thing that coerces your functions into async without adapting the corresponding calls is hilariously broken.

Absolutely write single-function unit tests, if the function has clearly defined inputs and outputs but a tricky implementation. A lot of business rules work this way, so if you have a "functional core, imperative shell" line-of-business application with unclear or changing requirements, unit tests will be your best friend for getting the details right.

But testing each class in the project with a Mockito-encrusted setup featuring cardboard bananas/gorillas/jungles—best practice in Java-land—seems tedious, and tedium is the enemy of a good testing strategy because programmers won't follow it. They'll bodge in a solution, get their teammates to lgtm it, and roll it out under the slightest bit of deadline pressure.

It also helps to not use languages with truthy / falsey values.

I kind of like the Scheme way of doing it, which is to have a true and a false value, and everything besides false is truthy. All strings including the empty string are true, all numbers including zero are true, all lists including the empty list are true. This makes it easy to define values as false when they are expected to be set to something else later, and you can just check the value as a condition to see if it's set or not, and then you can use it directly if it is.

IMO, the idea of trying to blur the line between client and server is a big mistake. I worked on WebSocket frameworks in the Node.js space so I was also tempted but I completely abandoned this approach years ago. Though with Node.js, I do often reuse utility functions between client and server, I reject any framework which tries to hide the separation. I demand to have complete understanding of where the code is executing and how. I need to know what is being executed, where and how.

I also avoid technologies where the code I write is different from the code being executed. This is why I've been avoiding TypeScript. It performs code transformations which obfuscate my understanding of the logic which will be executed. With TS, the code I write is not the code which gets executed. This is scary to me. My code gets compiled into some messy junk-looking code and then that messy junk gets executed in ways I don't fully comprehend.

You can use (ts 5.8 and above) a compiler flag `erasableSyntaxOnly` which enforces only TS features that can be dropped without modifying the underlying JavaScript. Unfortunately one of my favourite TS features, parameter properties, isn't erasable although it doesn't change runtime semantics.

> I also avoid technologies where the code I write is different from the code being executed.

Not to be snarky, but as opposed to writing assembly? Where do you draw the line if you don't allow TypeScript (which is, with limited exceptions, only type erasure unless you specifically request polyfill), but allow other forms of compilation? Would you define JVM bytecode or IR as messy-looking junk code?

It's hard to see how a principled line could be drawn here.

If I had the mental capacity and time to write it in assembly, then that would be better. But then probably still not worth it due to added maintenance challenges. I guess it's really about mental capacity and time.

The difference in the amount of value provided by a JS engine abstracting away from binary code vs TS transpiling to JavaScript is massive. The TS transpiler's value is extremely marginal by comparison, especially when considering the amount of trouble it brings.

Also, I'm familiar enough with the V8 engine and other ECMAScript-compliant engines that I have a good sense of how my code is being executed on there. I don't have to worry about TypeScript dropping support for a specific configuration setting in tsconfig and then having to spend hours trying to figure out why my code isn't working anymore.

I don't know next.js, can someone explain how the client side can call a server function inside of an IF on the client side for security? It seems like there would be a trivial bypass of the security from the client side.

It opaquely makes a network call. You call it from the client-side and it abstracts away the network round-trip, but inside the function context you're running code on the server.

Under the hood it opens up an endpoint and the function calls it via a HTTP request.

I am not arguing because I am ignorant and curious, but if "if (isOwner(userMail, resource.ownerMail)) { // grant access }" is on the client side, and the code for isOwner is async on server side as described, returning a promise waiting on the network call, isn't that trivial to hack?

The calling code is on the client. It's effectively a `await fetch('...generated-endpoint')` that looks like the code runs as part of the same process due to some type inference at dev time, but when you run a build it uses the `'use server'` marker described in the article to determine what to build out as server artifacts (i.e. lambdas).

You have to do your typical validations and auth logic on the server since anybody could call the endpoint without needing to use the function handle. There's a lot of magic and it seems to trip up a lot of people, including the people in the article.

Yes, this is my misunderstanding, if the calling code (client code) has the "if" with the secure block in the curly braces that should not be executed.

A client side attack can just jump over that server call in a variety of ways, from mitm to running in a debugger to patching it out.

It'd be no different from a fetch request. The inside of the function (the conditional) is run on a different machine with a different runtime process.

They're not asking "how does this work?", they're asking "how is this secure?".

The answer is, it's not. This is terrible code and should never be written like this - the check can be bypassed client side, letting the user act as an owner no matter what the response claimed.

This does not conflict with Andrew Kelly's point! The software is not your js code!! Your js code is s tiny fraction of the software. The rest of it (including node) has serious design issues. JavaScript is an messy interpreted dynamically-typed language with a week type system that was designed for browser 30 years ago and even its creator discourages you from using it now.

This posts intention was not to conflict with his point. I used it as a hook and because the simple perfect function he was showing reminded me of the one I mentioned in the article.

And I agree with you on the second half :).

It seems like the root cause was implicit conversion to boolean type. Stricter programming languages don't allow this. And even in statically-typed languages allowing this (C++) it's possible to disable such behavior via a compiler warning option.

always nice to read about these things. i like the note on 'all tests were green'. it sounds like the test of this function only test for the good case right? it should also test a false case? or am i missing something here?

We tested for both, the "happy" and the negative path. But the Javascript unit tests are run without the framework in between. So our function was returning expected results when run in isolation. Only when running it with Next.js the function became async which led to this dilemma.

> It is written as if this only applies to functions you mark with use server and not the whole file.

The demonstration code illustrating the problem uses a file-level "use server" directive (and doesn't have other functions in the same file with the problem isOwner function); if they were using function level "use server" directives and it impacted a different function in the same file, I would say this is clearly surprising and unexpected (and even buggy) behavior, but this seems to be using a clearly documented feature and getting exactly what is advertised.

> The snippet above was called as a server function. This is React's new way of calling server side code from the client side.

Tangential to the post, but mixing client-side and server-side code sounds like a recipe for disaster. There are already one too many services that perform authorization client-side, and I have a feeling making it harder to tell what runs where only makes the situation worse.

It's in the pursuit of type inference and developer ergonomics.

The problem without them in this day and age is how do you get your backend and frontend types in sync? You can manually update your types but this is tedious and prone to error (this is what we do at work), but having type errors immediately flagged on the client when you try to use a property wrong _is nice_.

The problem in this case is that it's so opaque that you have other issues like this one. Server Functions are unfortunately a low-level API in React and so their actual implementation is left up to frameworks.

Doesn't the 'use client'/'use server' directives tell you this?

This reminds me of functions that rely on properties, but the properties could, themselves, be functions (in Swift, we call them "computed properties," and they are indistinguishable, on the outside, from stored properties).

C++ can also have a lot of tripwires, from overloaded operators (same with Swift, but people don't overload operators very often, in Swift. They compute properties, all the time, though).

"The Power of 10 Rules" (2006, Gerard J. Holzmann)

https://en.wikipedia.org/wiki/The_Power_of_10:_Rules_for_Dev...

Generally wise advice. =3

And still there are coders who prefer non-statically typed languages, tsk tsk...

Explain how static typing would avoid this problem.

If you use the standard typescript linter, it will fail if you pass a Promise to an if statement.

https://typescript-eslint.io/rules/no-misused-promises/

The problem was the framework's bundler was transforming those functions to return promises, a linter would need to understand next.js's specific transforms to catch this.

A function returning a boolean cannot return a Promise. As simple as that!

Sure but if the framework also changes the signature of the function, there's nothing compiler can do. Unless you want it to catch `if(promise)`

My first thought was is JavaScript === case insensitive for string comps, because while I do use minimal JavaScript to enhance some web pages functionality it’s all basic vanilla JS.

But the answer is actually batshit crazy.

Oh man! How did these guys even debug?

Sometimes I feel I'm fighting an endless battle against opaque environments where simple things are slightly simpler and difficult things are incredibly hard because they're highly resistant to debugging tools.

I suspected it would be some feature of this moronic joke of a language and it seems I was right.

> When we called the isOwner function, it returned a Promise even though our function signature did not specify it as an async function. Our synchronous-looking function was invisibly converted into an async function. This meant it no longer returned a boolean, it returned a Promise.

Oh God.

I'll be down voted by JavaScript lobby for saying this but I'll still say this.

Never use JavaScript on the server side. The amount of bugs that can happen is insane. JavaScript is just a specification with varying implementations across various versions.

A proper language like Java, Go, or Rust ensures that your code will have fewer logical bugs of this type.

https://ashishb.net/tech/javascript/

this issue was caused by a framework that's trying to do too much, relying on "magic" interfaces to supposedly reduce developer burden. the function is very unambiguously written and the language did nothing wrong

I also support using whatever language you and your team prefer when you can. that's the glory of backend: no restrictions on what you can run. but sometimes you need to write client software, and those are strictly easier to manage in the platform's native tongue: Swift, Kotlin, JS, and so on

> this issue was caused by a framework that's trying to do too much, relying on "magic" interfaces to supposedly reduce developer burden. the function is very unambiguously written and the language did nothing wrong

The function is unambiguously written, but the runtime functions differently, and this is not a language problem? This is incoherent; one of these statements is incorrect.

> the function is very unambiguously written and the language did nothing wrong

The language absolutely did something wrong, by trying to evaluate a non-boolean type as a boolean. That is a horrible footgun and JS is absolutely at fault for doing so.

You'll be down voted appropriately for muddying the difference between the language runtime (on the server side there is one runtime people use and several small competitors) and the framework, which was the issue in this case

The fact it’s possible is the problem he’s pointing out.

That's nonsense. Code has bugs and you're always writing for a particular target runtime anyway. "Someone might run this on a version of the runtime that implements an incomplete or nonstandard version of the spec" is not a concern

That my compiler or interpreter may insert code that has a completely different outcome to the intended source code is way down the list in terms of thoughts when stumbling upon a bug.

I'd say this was React, not JS. And I do feel that all the back end conventions that have emerged around Node.js, etc. are a clusterfuck.

Can JS succeed in the back end? I say yes, but not like this.

My jaw dropped.

> Let's look at the following Javascript snippet:

Title checks out, say no more.

> The snippet above was called as a _server_ function.

A semantic shift in what a function is.. lovely. Is this Javascript or Python?

Reply on news.ycombinator.com