Error: AllowOrigin must be a quoted string when using !Sub/!ImportValue

[johnc44]

Description

Template uses !Sub to determine the CORS string to use. When using sam local start-pi the following error is thrown: b'Error: AllowOrigin must be a quoted string (i.e. "'value'" is correct, but "value" is not).\n'

Steps to reproduce

In template.yml specify AllowOrigin field to use !Sub, for example:

  AllowOrigin: !Sub
    - "'https://${FrontEndDomainName}.${dnszone}'"
    - {dnszone: !ImportValue mydomain-com-ZoneName}

Then sam build --use-container && sam local start-api

Observed result

Found '15' API Events in Serverless function with name 'myprojectApiLambda' Sending Telemetry: {'metrics': [{'commandRun': {'awsProfileProvided': False, 'debugFlagProvided': True, 'region': '', 'commandName': 'sam local start-api', 'duration': 135, 'exitReason': 'UserException', 'exitCode': 1, 'requestId': '226f246e-d2cb-4cd9-a496-a706b32e3284', 'installationId': '450286d8-42a1-4f22-9c69-81080b051b56', 'sessionId': '0d0e0c51-2097-4d57-9fb5-8569204975c9', 'executionEnvironment': 'CLI', 'pyversion': '3.7.2', 'samcliVersion': '0.22.0'}}]} HTTPSConnectionPool(host='aws-serverless-tools-telemetry.us-west-2.amazonaws.com', port=443): Read timed out. (read timeout=0.1) Error: AllowOrigin must be a quoted string (i.e. "'value'" is correct, but "value" is

Expected result

Should work...!

Additional environment details (Ex: Windows, Mac, Amazon Linux etc)

1. OS: OSX Mojave
2. sam --version: 0.22

[awood45]

Something I'm noticing here:

"'https://${FrontEndDomainName}.${dnszone}'"

The inside of the string would thus be:

'https://${FrontEndDomainName}.${dnszone}'

Is it intended to have two wrapping sets of quotes inside the YAML file?

[awood45]

Past this, it looks like we don't yet have built in support for Import Values for sam local - noting the feature request.

[johnc44]

Something I'm noticing here:

"'https://${FrontEndDomainName}.${dnszone}'"

The inside of the string would thus be:

'https://${FrontEndDomainName}.${dnszone}'

Is it intended to have two wrapping sets of quotes inside the YAML file?

Yes, that seems to be the syntax for some peculiar reason. E.g. an example I found on Stack Overflow:

Globals: Api: Cors: AllowOrigin: "'*'"

I remember when we originally put this code in, we had to put the extra set of quotes in there.

Correct me if I'm wrong, but I'm pretty certain it won't work at all without the extra set of quotes.

[johnc44]

For some reason I couldn't seem to find it earlier, but this is the official sam doc which mentions the extra quotes: https://github.com/awslabs/serverless-application-model/blob/master/versions/2016-10-31.md#cors-configuration

For example, "'www.example.com'".

[johnc44]

@awood45 - noticed this has "more info needed" tag - are you needing anything further from me?

I also don't think this should be classified as a feature request as it worked in previous versions. (I'm personally still stuck on 0.16 until both this and the MFA bugs are fixed)

[jfuss]

@johnc44 The issue you are seeing is because we cannot resolve the !Sub completely and therefore we leave it as the Sub. My guess on why we can't resolve would be due to the ImportValue !ImportValue mydomain-com-ZoneName. We do not have a way to get values for other stacks.

I think @awood45 probably didn't realize that you need the double quotes, per the spec.

I would love to hear your thoughts on how to go about addressing things like ImportValue that we have a hard time resolving. The best workaround I can think of right now is to create some condition and pass a value into a Parameter for when you do local dev. I completely realize this isn't the best thing though, as it requires you to maintain the template to workable locally.

Sidenote on the MFA: I am waiting for a build to pass. You can then use brew install aws-sam-cli --head to install the HEAD of develop, which should address that MFA issue for you. PR is #1494 for reference.

[johnc44]

Thanks, @jfuss - after writing the above I did discover that other PR. That's great news - thanks.

Regarding the CORS thing, I'm not 100% sure I understand. At one point it worked. I only looked at this briefly at the time, but my impression was that the change was to put some validation logic in there and explicitly throw an error if the string isn't in the format you require.

Is this logic for testing locally or is it needed for deployments too? If it's testing locally, can anythin g unknown just be ignored for now?

[johnc44]

@jfuss - I had a quick look at the code. In _get_cors_prop inside sam_api_provider.py it explicitly checks to see if a property is a string and that it is enclosed in quotes. If either of these checks fail, then an exception is raised. Can this bit of code be changed so that if the property is not a string, then it returns None as if the property wasn't there?

Is there any particular reason this check needs to be there? I presume it's just a helpful nudge in case someone forgets the quotes?

[johnc44]

@jfuss - I forked the repo and made the change (https://github.com/johnc44/aws-sam-cli/blob/74c8a0a06a26b7ee08a451e39d0322ef3a22cc90/samcli/lib/providers/sam_api_provider.py#L150)

From a quick test, I could start the API up and all the unit tests pass. I've not really had the time to setup the environment correctly, write extra tests or even try deployments or anything else with it.

I'm reasonably sure this code only exists to support local CORS, and it's trying to be helpful by suggesting that quotes are missing. So by just ignoring anything SAM doesn't understand, then it means it can work in the way it used to (i.e. with no local CORS support).

I think in an ideal world, I would be able to use different CORS values locally anyway. If I'm running my front-end and API both locally, then I want to use localhost CORS, not the CORS I've setup in my CloudFormation template would never match that. I'm probably missing something obvious, but I can't see how the current implementation meets any use cases without manually having to change the template for dev? Isn't the CORS header always going to be environment specific?

Slightly off-topic, but having a way for SAM to be able to parse this sort of thing would be useful as it's a pain (albeit an understandable one) that SAM doesn't cope with !ImportValue for environment variables.

[johnc44]

@jfuss - was having another look at this, and think the answer is actually very simple. If the CORS header refers to a dictionary or a cfn function (e.g. !Ref) then return '*'. That way CORS works locally fine. It'll need checking when deployed to AWS, but that would be the case anyway. I've updated my fork after rebasing to the latest 0.44 and it seems to work (I even managed to get rid of my CORS proxy).

It would be great to get this fixed so I can finally get us onto the latest official releases.

Still not able to get integration tests to run though so struggling to progress it atm. (But at the same time not had a huge amount of time to spend on it)

[scotttelle]

Relatedly I am trying to conditionally enable cors in specific environments and ran into this issue. My template deploys to AWS fine, but I can not run sam local.

Parameters:
  Environment:
    Type: String
    Default: dev
...

Conditions: 

  IsDev: !Equals [ dev, !Ref Environment ]

Resources:
  UsersAPI:
    Type: AWS::Serverless::Api
    Properties:
      ....
      Cors:
        AllowHeaders: !If
        - IsDev
        - "'Cache-Control, Pragma, Origin, Authorization, Content-Type, X-Requested-With'"
        - !Ref AWS::NoValue
        AllowOrigin: !If
        - IsDev
        - "'http://localhost:3000'"
        - !Ref AWS::NoValue

[jfuss]

@johnc44 "If the CORS header refers to a dictionary or a cfn function (e.g. !Ref) then return '*'."

I don't think we want this. This makes a huge assumption on the value and can lead to "this worked locally but not in the cloud". We might want a way to pass these values into the command so we can resolve things correctly. This puts additional work on getting the command together but then we (the CLI) are not guessing at what the customer expected.

[johnc44]

@jfuss - I don't really agree with this.

At the moment I'm in the situation where it doesn't work at all locally, so I either have to hack my yaml temporarily while I test - and hope I don't accidentally commit it this way - or I have to always deploy and test remotely, which defeats the main advantage of using SAM.

I would also say that if you aren't testing things like this remotely, then you are asking for trouble. Normally with CORS I would think you set it once, and when it's right you don't touch it again. But it will be different for every environment and therefore needs to be tested in every environment.

I think the choices are:

1. Fully support all CloudFormation fuctions like !Ref, !Sub, !ImportValue including combinations therefore. Even this wouldn't be perfect because any environment vars setup in my cloudformation won't really apply locally, so I'd still end up having to use a proxy.
2. When testing locally, use * and just allow it to work, albeit with a caveat.
3. Allow CORS headers to be overridden somehow - use using a commandline flag, or something in environment.json etc. (This would be fine as long as it was easily discoverable)
4. When testing locally, don't set CORS headers at all, and I will continue to use my CORS proxy. This is pretty much how it used to work until the breaking change was added.

Surely the current situation requires that you only have one environment (and therefore one hardcoded CORS value) and then use a CORS proxy to change the header when testing locally. Is this in any way representative of how someone is going to use this?

It's particularly frustrating because it's clearly a regression that was introduced months ago and it seems very easy to at the very least put it back to how it was. It's obvious that the change was well-intentioned but it's just not fit for any purpose as far as I can tell.

[jfuss]

@johnc44 I understand this is frustrating and I apologize for this. I do not think we should magically set values, this can have other implications and while you understand this detail others may not. This will lead to you moving on but others get into a situation where they don't understand what is going on.

On #1, we do our best effort to do that. When we cannot resolve everything locally and have taken the stance on leaving the intrinsic alone and this has the least risk and matches what we did in the past when we did not resolve any intrinsic functions at all.

I think allowing some way to override the Intrinsic value or property, something similar to/expand our --env-var, would be reasonable to me. You may be able to do with this already by adding FN:If instrinsics to your template to allow locally switching (I have not tested but pretty sure we support those already).

[johnc44]

@jfuss - can we not just remove the breaking change until it can be done properly?

[jfuss]

@johnc44 Which is what? All of CORS or all of Intrinsics?

[johnc44]

@jfuss A change was made in SAM which specifically checks for the CORS value to be surrounding in quotes. If it's not, it raises an error. With that check removed, it works well enough to be usable locally with a CORS proxy. That's all that would need to be changed for me to get up and running again I believe.

My original change basically just tried to handle that case. It was only my subsequent change which altered it to return "*" which meant I didn't need to use a CORS Proxy. But as long as it's usable then I can live with it. I think using CORS locally has been raised elsewhere.

[jfuss]

@johnc44 Sorry I was missing understand some piece here. Long term I think we still need a better way to solve these types of things. I know its incredibly frustrating on your (and others) part. I am ok moving this from an Error to some kind of log warning to allow this until we have a better solution. Let me raise it with the team (to make sure everyone is aware and understands).

[johnc44]

@jfuss - that would be great, thank you. If the check is still valuable, it can still be done, but just not for dictionaries or strings starting !. (If you look at the change I posted above, that's what I was trying to achieve)

[scotttelle]

@jfuss @johnc44 I agree - a warning would be very appropriate for this issue. It's frustrating trying to troubleshoot what's going wrong with CORS and I see the need for this check to ensure the template is correct! Thank you so much for being responsive and communicating the updates @jfuss!

[jfuss]

@jfuss @scotttelle A patch to log a warning is now in develop and should be released with the next version.

[johnc44]

@jfuss - tried the latest version and it looks like this problem has now been fixed. (I noticed the warning too).

Thank you!!! I can finally get upgraded to the latest version.

I think in an ideal world it'd be nice if I could avoid the need for the proxy to get cors to work locally, but I have a quite usable solution now so I'm happy.

[jfuss]

Last comment says the problem has been fix and since there hasn't been any follow up since then, going to assume we (I) just missed closing this.

Closing.

[github-actions[bot]]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.