search

commercialhaskell / stackage

Stable Haskell package sets: vetted consistent packages from Hackage
https://www.stackage.org/
MIT License
525 stars 797 forks source link

Flags used to pass tests become set for all installations #7438

Open pbrisbin opened 1 month ago

pbrisbin commented 1 month ago

It seems in the past, we decided to set a development:true flag in one of our packages for lts-22.23.

This flag is necessary for the tests to pass, and test failures is what prompted us to do this. It all seemed reasonable at the time, but we had no idea that setting this flag there, to get tests passing on Stackage CI, also meant that everyone installing the package from Stackage ever would build the package with this flag set. Had anyone thought about this, obviously a flag named development should probably not be used this way.

The result is Very Bad. The flag breaks the security of the package, causing it to validate SNS webhooks as coming from a localhost-signed certificate (which we use in tests) rather than an AWS-signed certificate. If we shipped a service with this library built this way, someone could submit a self-signed payload and it would be erroneously accepted as if it were AWS-signed.

I'm opening this Issue with two questions:

  1. What is the best thing to do about this now, such that installs from lts-22.23 no longer get a development:true-built version of aws-sns-verify-0.0.0.3?

  2. The situation in general seems like something that should be changed. Do you agree?

    It seems totally reasonable, and common, that packages might use flags to change how they work in development/test, that such flags would be needed to pass tests when Stackage CI runs it, but such flags would NOT be needed (and could be harmful) when users install those packages. It thus seems really bad to conflate the flags used to test the packages in Stackage with the flags used when users install from there.

juhp commented 4 weeks ago

You are right - I think flags should only be set if they are to be used by end-users: I guess we gave been lucky that this hasn't been a serious problem until now - at least that this was a potential problem had not even occurred to me.

I guess in this case we should drop the flag and allow the tests to fail or disable them as needed.

I don't really think there is much we can do for the already released snapshots (lts-22.23 and lts-22.24), but we should do that from lts-22.25 at least.

juhp commented 4 weeks ago

I note the same is true for nightly too.

Also we should document this somewhere: probably in build-constraints.yaml

pbrisbin commented 4 weeks ago

Thanks @juhp let me make sure I understand:

The plan is to document the fact that the flags in build-constraints.yaml are meant to specify flags that are to be used all the time by end-users and not flags specifically to influence Stackage CI?

That seems a bit odd -- why wouldn't such a use-case be better-served by a different default in the project's cabal file?

juhp commented 3 weeks ago

@pbrisbin, I didn't fully understand your suggestion. But if you are thinking there should be more than one flags sections in Stackage, I don't see how that would work. The thing is Stackage is basically just one huge stack build, so there's no way currently to use such configuration.

Also personally I am becoming of the opinion lately that we maybe should consider completely stopping running testsuites in our Stackage builds: they actually take up a considerable amount of the total human time spent on Stackage work... I feel just building the testsuites but not running them might be a saner sweet spot for Stackage.

pbrisbin commented 3 weeks ago

But if you are thinking there should be more than one flags sections in Stackage, I don't see how that would work. The thing is Stackage is basically just one huge stack build, so there's no way currently to use such configuration

Yeah, I wasn't ready to suggest something concrete. It just seemed that "flags to use when Stackage tests this" and "flags to use whenever installed from this resolver" are two very different use cases. And I'd claim the latter is simply not something anyone needs.

I admit I don't know the ins-and-outs, it just seemed like somewhere build-constraints.yaml gets used and somewhere else lts/X.Y.yaml is produced. Why can't the flags in the former be used when testing, but not propagate to the latter? That would solve things, IMO.

I feel just building the testsuites but not running them might be a saner sweet spot for Stackage

Complete agreement here.