SIWECOS / HSHS-DOMXSS-Scanner

MIT License
3 stars 1 forks source link

HEADER information and wrong fixes / Verschlimmbessern #17

Closed Lednerb closed 6 years ago

Lednerb commented 6 years ago

@Weegy please group and name your issues in a better way, so there are not 15 commits with the same comment in the future.


Also please test your Fixes so no new errors occur: https://github.com/SIWECOS/HSHS-DOMXSS-Scanner/blob/aedb7ac266b1748c58d84af24f4e510caaf7f496/app/Ratings/XFrameOptionsRating.php#L25-L26

The placeholder value is wrong;


Why does the XFrameOptionsRating not include the original HEADER? Also some Ratings include the $header and some not - and not consistent.

Doesn't checked the other ones for further errors.


In general: Why does it make sense for you to send the header for every test again and not send the header once... I don't get it - why does each header test cannot simply contain the actually set header so the siwecos user can see the actual set header and the best practice one side by side in the frontend?

Weegy commented 6 years ago

@Lednerb Sounds good, but before blaming others, start over to do your stuff right, so no other have to deal with it...

Kind regards

jurajsomorovsky commented 6 years ago

@Weegy could you please elaborate on the stuff @Lednerb should do right?

I mean he provided you a constructive issue. Typical way to handle such github issues and questions is to answer them. If you have any issues @Lednerb should solve, please submit them, including details, similarly as provided in this issue.

Skeeve commented 6 years ago

@jurajsomorovsky please see https://github.com/SIWECOS/HSHS-DOMXSS-Scanner/issues/16 as an example what was wrong.

You see: We once agreed(!) on an API which every scanner has to conform to.

If one scanner programmer thinks that this is inefficient and that there is a better way to transfer the information he should not simply do it, but discuss with everyone else and agree on a new API.

As a matter of fact, these scanners were the only ones where we couldn't use the results adn @Weegy just made it conform to the API.

Lednerb commented 6 years ago

(For better understanding, I use the Content-Type-Test here)

It's right, that the first implementation didn't follow the API and as @Skeeve requested in https://github.com/SIWECOS/HSHS-DOMXSS-Scanner/issues/16#issuecomment-380008075

Expected is something like:

 "testDetails": [{
   "placeholder": "HEADER",
   "values": {
      "type":"text\\/html; charset=utf-8",
      "site":"https://your.do.main"
    }
  }, {
    "placeholder": "CT_CORRECT"
  }]

, I've adjusted the testDetails here: https://github.com/SIWECOS/HSHS-DOMXSS-Scanner/commit/ff3f1fcc4390b8f28f211ff695ca8755fbc78629#diff-11b84d0d42ab0349aaddc2939e26ec9e


Furthermore, the whole HSHS-DOMXSS-Scanner does not have any placeholders in its messages as the other scanners have. All messages are listed here: https://github.com/SIWECOS/HSHS-DOMXSS-Scanner#scanner-interface-values

So the testDetails array is never used besides in the HEADER (for every test) and META (only for the Content-Type test) sections.

These two sections were added to the JSON results, so it's possible for the user that scanned his site on siwecos to compare the actual set header on his site with the best practice ones. This feature was requested (and it's actually not used anyway), so I've implemented the HEADER as a placeholder that can be replaced with the complete values entry. That is the same behavior as all the other placeholders (for example CT_CORRECT) are handled.


I've never tried or requested to change the defined API. I've instead implemented the HEADER placeholder in the same way the other text messages are implemented.

Maybe this was a wrong decision, so there was issue #16, and I tried to fix that according to the response with the code snipped quoted above.


This issue is about the strange commit method and the verifiable inconsistent implementation of @Weegy's TEST FIX HEADER commits.

For example, the Content-Type test does include the HEADER and META values (that's correct!), but the Strict-Transport-Security test does not include the HEADER value.


All in all, there is my conclusion:

Kind regards.

Weegy commented 6 years ago

@Lednerb Sorry for my words answering your first post. I know, I was not working in a BP way with this commits. But as you know it was only as short amount of time left. I will make a Rollback from my commits and prepare a proper commit. We should all calm down :)

Cheers Marcel

Lednerb commented 6 years ago

done by #20