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

Reduce security risk on remote_census_api #3784

Merged
merged 3 commits into from
Oct 23, 2019

Conversation

taitus
Copy link
Member

@taitus taitus commented Oct 21, 2019

References

Related PR: #3498
Related Documentation PR: Update remote census doc #86

Objectives

The use of eval is a serious security risk, so we change by JSON.parse method.
Describe expected format for Setting["remote_census.request.structure"]

Visual Changes

Captura de pantalla 2019-10-21 a las 17 39 38

Notes

⚠️ If your installation is using the "Remote Census configuration" feature and you have already configured the "Request Structure" field, please be sure to update the format of this field.
You can find this section on: Settings > Global Settings > Remote Census configuration

Example for old expected format value:

{
  request: {
    codigo_institucion: 12,
    codigo_portal: 5,
    documento:   ,
    tipo_documento:  ,
  }
}

Example for new expected format value:

{
  "request": {
    "codigo_institucion": 12,
    "codigo_portal":  5,
    "documento":  null,
    "tipo_documento":  null,
  }
}

The use of eval is a serious security risk, so we change by JSON.parse method
@javierm javierm added the security Pull requests that address a security vulnerability label Oct 21, 2019
@javierm javierm added this to Reviewing in Roadmap via automation Oct 21, 2019
@javierm javierm changed the title [WIP] Improve security risk on remote_census_api [WIP] Reduce security risk on remote_census_api Oct 21, 2019
@@ -84,7 +84,7 @@ def client
end

def request(document_type, document_number, date_of_birth, postal_code)
structure = eval(Setting["remote_census.request.structure"])
structure = JSON.parse(Setting["remote_census.request.structure"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since now we aren't using eval anymore 🎉, would it be possible to move the Security/Eval rubocop rule from .rubocop.yml to .rubocop_basic.yml (where rules are in alphabetic order, by the way)? That way our dear Hound will report about it 😄.

lib/remote_census_api.rb Show resolved Hide resolved
config/locales/en/settings.yml Show resolved Hide resolved
@javierm javierm changed the title [WIP] Reduce security risk on remote_census_api Reduce security risk on remote_census_api Oct 21, 2019
@javierm javierm moved this from Reviewing to Doing in Roadmap Oct 21, 2019
@javierm javierm moved this from Doing to Reviewing in Roadmap Oct 22, 2019
@javierm javierm self-assigned this Oct 22, 2019
Roadmap automation moved this from Reviewing to Testing Oct 23, 2019
Copy link
Member

@javierm javierm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 🎉

@javierm javierm merged commit 3a0871d into consuldemocracy:master Oct 23, 2019
Roadmap automation moved this from Testing to Release 1.1.0 Oct 23, 2019
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
…ity-risk

Reduce security risk on remote_census_api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Pull requests that address a security vulnerability
Projects
No open projects
Roadmap
  
Release 1.1.0
Development

Successfully merging this pull request may close these issues.

None yet

2 participants