Maybe use error logging and/or exceptions instead of `dd`?

[andrewminion-luminfire]

We’re using the CMS in an app that requires static code analysis scan. The scanner is reporting usage of dd as active debug code as in CWE-489

Any thoughts about changing that to Log::error() and/or throwing Exceptions instead of using dd? I’d be happy to test and submit a PR.

[OwenMelbz]

You able to show what code it’s referring to please?

Thanks

[andrewminion-luminfire]

Sorry…I had copied the links and just forgot to include them. Here:

• https://github.com/maelstrom-cms/toolkit/blob/e30edb93af3ad814e55b20f912151819a9b1d4d4/src/Panel.php#L416
• https://github.com/maelstrom-cms/toolkit/blob/e30edb93af3ad814e55b20f912151819a9b1d4d4/src/Panel.php#L1334
• https://github.com/maelstrom-cms/toolkit/blob/e30edb93af3ad814e55b20f912151819a9b1d4d4/src/Panel.php#L1400

[OwenMelbz]

Right okay.

So this is probably a bit tricky situation because...

The weakness your linking is saying if it’s accidentally left in there which could leak sensitive information.

However it’s not accidentally left there and not leaking sensitive information.

Thus the problem it is highlighting would be a false positive after assessment.

——

Ironically throwing an exception is MORE likely to reveal sensitive information if a stack trace or whoops/ignition/xdebug or something displays more information, so the dd is more controlled.

——-

The reason these are dd and not exceptions is because they’re not really exceptions in the case of something’s broken, they’re there as developer hints which work across debugging levels to help the dev identify what they’re doing whilst building it. They should never get into production because the code has been changed by then.

——-

Now, I know those answers don’t help your situation!

Cloud you extend the Panel class and use your one, then throw exceptions or do what you like?

Maybe you can suppress or mark as revolved somehow?

[andrewminion-luminfire]

Ironically throwing an exception is MORE likely to reveal sensitive information if a stack trace or whoops/ignition/xdebug or something displays more information, so the dd is more controlled.

That makes perfect sense, though I believe whoops/ignition/etc. is disabled in production apps?

I’ll mark them as mitigated by design but if I get pushback, I can extend the class to mitigate it.

Thanks!

[OwenMelbz]

Oh yeah, you would hope it is disabled!! Number of sites I’ve come across where errors have been left on is painful!

Good luck with the rest of the audit!

Thanks