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

custom REST CORS header configuration is not applied to responses #26391

Closed
yolabingo opened this issue Oct 6, 2023 · 4 comments · Fixed by #26511
Closed

custom REST CORS header configuration is not applied to responses #26391

yolabingo opened this issue Oct 6, 2023 · 4 comments · Fixed by #26511
Assignees
Labels
Doc : Needs Doc Note to QA OKR : Customer Success Owned by Arno OKR : Customer Support Owned by Scott QA : Approved Release : 22.03.11 Included in LTS patch release 22.03.11 Release : 23.01.8 Included in LTS patch release 23.01.8 Release : 23.10.24 v1 Included in LTS patch release 23.10.24 v1 Release : 23.12.21 SEO Improvements Team : Bug Fixers Type : Defect

Comments

@yolabingo
Copy link
Contributor

yolabingo commented Oct 6, 2023

Parent Issue

No response

Problem Statement

CORS header config changes are not working - the default headers remain unchanged.

Custom CORS header config like

        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-CREDENTIALS: 'false'
        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-ORIGIN: 'http://localhost'
        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-METHODS: 'POST'

has no effect on the default headers

Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,PUT,POST,DELETE,HEAD,OPTIONS,PATCH
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: *
Access-Control-Expose-Headers: *

I found that setting global DEFAULT headers appeared to work on 5.3.8, but per-resource headers may not.

DOT_API_CORS_CONTENTRESOURCE_ACCESS-CONTROL-ALLOW-ORIGIN: 'http://example.com'

Steps to Reproduce

Fetch default headers
curl -ks -X OPTIONS --head http://127.0.0.1:8082/api/v1/contenttype | grep -i access-control

Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,PUT,POST,DELETE,HEAD,OPTIONS,PATCH
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: *
Access-Control-Expose-Headers: *

Add Docker environment variables such as

        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-CREDENTIALS: 'false'
        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-ORIGIN: 'http://localhost'
        DOT_API_CORS_DEFAULT_ACCESS-CONTROL-ALLOW-METHODS: 'POST'

Restart - dotCMS CORS header response does not change.

Acceptance Criteria

REST CORS headers should be configurable.

It would be nice extend this functionality as well, for example being able to support multiple Origin hosts.

dotCMS Version

LTS 21.06 22.03 23.01 and agile

It appears this worked, at least partially, in 5.3.8.

Proposed Objective

Core Features

Proposed Priority

Priority 2 - Important

External Links... Slack Conversations, Support Tickets, Figma Designs, etc.

No response

Assumptions & Initiation Needs

No response

Quality Assurance Notes & Workarounds

No response

Sub-Tasks & Estimates

No response

@yolabingo
Copy link
Contributor Author

Cloud support ticket https://dotcms.zendesk.com/agent/tickets/113022

@erickgonzalez
Copy link
Contributor

erickgonzalez commented Oct 12, 2023

Note To QA and Doc

Let's now add the properties like:
DOT_API_CORS_DEFAULT_ACCESS_CONTROL_ALLOW_CREDENTIALS

This means using an underscore instead of a hyphen.

erickgonzalez added a commit that referenced this issue Oct 12, 2023
@yolabingo
Copy link
Contributor Author

yolabingo commented Oct 12, 2023

A quick test looks good for DEFAULT - setting

        DOT_API_CORS_DEFAULT_ACCESS_CONTROL_ALLOW_CREDENTIALS: 'false'
        DOT_API_CORS_DEFAULT_ACCESS_CONTROL_ALLOW_METHODS: 'PUT'
        DOT_API_CORS_DEFAULT_ACCESS_CONTROL_ALLOW_ORIGIN: 'http://example.com/'

returns

Access-Control-Allow-Methods: PUT
Access-Control-Allow-Credentials: false
Access-Control-Allow-Origin: http://example.com/
Access-Control-Allow-Headers: *
Access-Control-Expose-Headers: *

I found that the example in the docs for a
Resource Specific Header does not appear to work - trying to set headers for "content" resource

        DOT_API_CORS_CONTENTRESOURCE_ACCESS_CONTROL_ALLOW_CREDENTIALS: 'false'
        DOT_API_CORS_CONTENTRESOURCE_ACCESS_CONTROL_ALLOW_METHODS: 'PUT'
        DOT_API_CORS_CONTENTRESOURCE_ACCESS_CONTROL_ALLOW_ORIGIN: 'http://example.com/'

does not change the default headers for
curl -ks --head -X OPTIONS http://localhost:8082/api/content

Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,PUT,POST,DELETE,HEAD,OPTIONS,PATCH
Access-Control-Allow-Credentials: true
Access-Control-Allow-Headers: *
Access-Control-Expose-Headers: *

New ticket created for specific resources: #26503

@erickgonzalez erickgonzalez linked a pull request Oct 24, 2023 that will close this issue
@erickgonzalez erickgonzalez added the Release : 23.01.8 Included in LTS patch release 23.01.8 label Nov 7, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2023
* fix: pull system envs for cors ref#26391

* test: add test for config changes ref: #26391
@erickgonzalez erickgonzalez reopened this Nov 7, 2023
@erickgonzalez erickgonzalez added Doc : Needs Doc Release : 22.03.11 Included in LTS patch release 22.03.11 labels Nov 8, 2023
erickgonzalez added a commit that referenced this issue Nov 9, 2023
@erickgonzalez erickgonzalez added LTS : Next Ticket that will be added to LTS Note to QA and removed Next LTS Release labels Nov 9, 2023
@josemejias11
Copy link

Approved: Tested on master_39ca5c4_SNAPSHOT, Docker, macOS 13.0, FF v113.0
Screenshot 2023-11-21 at 2 20 31 PM

erickgonzalez added a commit that referenced this issue Dec 5, 2023
erickgonzalez added a commit that referenced this issue Dec 5, 2023
@erickgonzalez erickgonzalez added Release : 23.10.24 v1 Included in LTS patch release 23.10.24 v1 and removed LTS : Next Ticket that will be added to LTS labels Dec 5, 2023
@erickgonzalez erickgonzalez added LTS: Excluded Ticket that has been excluded from at least one LTS and removed LTS: Excluded Ticket that has been excluded from at least one LTS labels Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc : Needs Doc Note to QA OKR : Customer Success Owned by Arno OKR : Customer Support Owned by Scott QA : Approved Release : 22.03.11 Included in LTS patch release 22.03.11 Release : 23.01.8 Included in LTS patch release 23.01.8 Release : 23.10.24 v1 Included in LTS patch release 23.10.24 v1 Release : 23.12.21 SEO Improvements Team : Bug Fixers Type : Defect
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants