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

Unable to create ns1_redirect resource when target url contains query string parameters #326

Closed
AlejandroTrev opened this issue Aug 22, 2024 · 3 comments · Fixed by #327
Closed

Comments

@AlejandroTrev
Copy link

AlejandroTrev commented Aug 22, 2024

Hi there,

Using the ns1_redirect resource that was introduced in #307 (released in v.2.3.0), I'm unable to create a redirect where the target url contains query string parameters.

Terraform Version

terraform -v
Terraform v1.9.5
on darwin_arm64
+ provider registry.terraform.io/ns1-terraform/ns1 v2.4.0

Affected Resource(s)

  • ns1_redirect

Terraform Configuration Files

terraform {
  required_providers {
    ns1 = {
      source  = "ns1-terraform/ns1"
      version = "~> 2.4.0"
    }
  }
}

provider "ns1" {
  apikey = "YOUR_API_KEY_HERE"
}

resource "ns1_redirect" "foo_testingzone_com_redirect" {
  domain           = "foo.testingzone.com"
  path             = "/"
  target           = "https://www.google.com/?foo=bar"
  forwarding_mode  = "none"
  forwarding_type  = "permanent"
  query_forwarding = false
}

Expected Behavior

The plan should indicate that the redirect will be created.

Actual Behavior

An error message is returned:

│ Error: target is not a valid FQDN, got: https://www.google.com/?foo=bar
│
│   with ns1_redirect.foo_testingzone_com_redirect,
│   on main.tf line 22, in resource "ns1_redirect" "foo_testingzone_com_redirect":
│   22:   target           = "https://www.google.com/?foo=bar"

Steps to Reproduce

  1. terraform plan

Important Factoids

The target field uses the ValidateFunc validateURL defined here: https://github.com/ns1-terraform/terraform-provider-ns1/blob/master/ns1/resource_redirect.go#L373

The regex should be updated to allow query string parameters in the url. NS1 supports target urls with query string parameters when creating the redirects through their admin console. Example:

redirects

@AlejandroTrev
Copy link
Author

@fformica just a heads up about this one, since you worked on #307

@fformica
Copy link
Contributor

Hi @AlejandroTrev, thanks for the heads-up!
We need to improve this regex:

match, err := regexp.MatchString("^(http://|https://)?[a-zA-Z0-9\\.\\-/$!+(_)' ]+$", v)

However I also tried creating a redirect with that target and when I curl to it I see

* Request completely sent off
< HTTP/1.1 301 Moved Permanently
< location: https://www.google.com/

So it looks like the query part is currently ineffective: we'll have to do an investigation server side as well.

@AlejandroTrev
Copy link
Author

However I also tried creating a redirect with that target and when I curl to it I see

* Request completely sent off
< HTTP/1.1 301 Moved Permanently
< location: https://www.google.com/

So it looks like the query part is currently ineffective: we'll have to do an investigation server side as well.

Yes, you are correct! I have a support ticket open with NS1 about this (sorry I forgot to mention it). They advised me to url-encode the target url, e.g. https://www.google.com/%3Ffoo%3Dbar, which I tested and seems to work. That being said, their admin console (and by extension their API) accepts human-readable urls, so they need to improve their input validation there. I also gave them the feedback that they should accept human-readable urls, and just handle url-encoding on the backend.

fformica added a commit that referenced this issue Aug 23, 2024
* Review redirect validation regexes

* Add unit test check

* Re-enabling import verification
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 a pull request may close this issue.

2 participants