Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] survey: Don't disclose the survey via print. #30166

Merged

Conversation

mvaled
Copy link
Contributor

@mvaled mvaled commented Jan 12, 2019

The contents of a survey could be disclosed through the print method. This PR fix this issue.

I read the Responsible Disclosure of Odoo Security Vulnerabilities document, and I found this issue not be worthy of the process described there.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@mvaled
Copy link
Contributor Author

mvaled commented Jan 12, 2019

The CLA of Larisa was signed in a previous PR #29737, but that has not been merged yet.

@mart-e mart-e force-pushed the 10.0-fix-survey-info-disclosure branch from 598e8fc to 7deb219 Compare January 14, 2019 15:22
@mart-e
Copy link
Contributor

mart-e commented Jan 14, 2019

Hi,

It looks good to me but just to be sure, @rim-odoo do you agree too?

@mart-e
Copy link
Contributor

mart-e commented Jan 15, 2019

Hi again,

Sorry but actually, your patch introduces a change of behaviour (which should not happen in a stable version), you could no longer print a closed survey.

Instead, we could only check if the survey has auth_required enabled and make sure the user is not public.
Could you update?

@mvaled
Copy link
Contributor Author

mvaled commented Jan 15, 2019

Hi @mart-e

I propose to add a 'allow_closed' argument to the method _check_bad_cases. So can still use it but allow to print closed surveys. By default it keeps the old behavior.

Is that acceptable?

@mvaled mvaled force-pushed the 10.0-fix-survey-info-disclosure branch from 7deb219 to bf165d3 Compare January 15, 2019 13:37
@mvaled
Copy link
Contributor Author

mvaled commented Jan 15, 2019

Sorry I didn't realized you forced a push before. I will rebase onto 10.0.

@mvaled mvaled force-pushed the 10.0-fix-survey-info-disclosure branch from bf165d3 to 88031c8 Compare January 15, 2019 13:40
@mart-e
Copy link
Contributor

mart-e commented Jan 15, 2019

We try not to change the signatures of methods in stable versions.
What do you wish to prevent exactly? Isn't the auth part enough?

We know the survey module is not the best and we plan to refactor it in the future but in 10.0, we try to keep the changes to the minimal (it is an end of life version).
If you want to make larger changes, it should probably target the master version instead.

@mvaled mvaled force-pushed the 10.0-fix-survey-info-disclosure branch 2 times, most recently from 4001148 to a397fcd Compare January 15, 2019 16:27
@mvaled
Copy link
Contributor Author

mvaled commented Jan 15, 2019

Ok. Changed.

@robodoo robodoo added the CI 🤖 Robodoo has seen passing statuses label Jan 15, 2019
@MiquelRForgeFlow
Copy link
Contributor

@tde-banana-odoo

To avoid bigger changes, only verify if the login is required.
A proper refactoring has been implemented in master at 9771fdf
@mart-e mart-e force-pushed the 10.0-fix-survey-info-disclosure branch from a397fcd to da74758 Compare January 16, 2019 09:39
@mart-e
Copy link
Contributor

mart-e commented Jan 16, 2019

Thanks for the update !

robodoo r+

@robodoo robodoo added r+ 👌 and removed CI 🤖 Robodoo has seen passing statuses labels Jan 16, 2019
robodoo pushed a commit that referenced this pull request Jan 16, 2019
To avoid bigger changes, only verify if the login is required.
A proper refactoring has been implemented in master at 9771fdf

closes #30166
@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses merging 👷 and removed CI 🤖 Robodoo has seen passing statuses merging 👷 labels Jan 16, 2019
@robodoo
Copy link
Contributor

robodoo commented Jan 16, 2019

Staging failed: ci/runbot on a774b02dbe4d07ed8b1d1210e9acf025c9dc241f (view more at http://runbot.odoo.com/runbot/build/428749)

@mart-e
Copy link
Contributor

mart-e commented Jan 16, 2019

robodoo retry

@robodoo robodoo added CI 🤖 Robodoo has seen passing statuses r+ 👌 and removed CI 🤖 Robodoo has seen passing statuses merging 👷 labels Jan 16, 2019
@robodoo
Copy link
Contributor

robodoo commented Jan 16, 2019

Staging failed: ci/runbot on d355ccc202296b532ba128859785487acc11e1a2 (view more at http://runbot.odoo.com/runbot/build/428810)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants