diff options
| author | skarbek <jskarbek@gitlab.com> | 2022-07-28 19:37:51 +0300 |
|---|---|---|
| committer | skarbek <jskarbek@gitlab.com> | 2022-07-28 19:37:51 +0300 |
| commit | 5bf8f67527377440f381981fc0fcd900fb832ca3 (patch) | |
| tree | 70fce8667cfe1242f17357ac62b2d8be559f6bad | |
| parent | a80c58682c3c313f52deecdebecfef2d1f7e3b21 (diff) | |
| parent | 74f53f4c5a5c81a65e3666ef6788d14d2de23588 (diff) | |
Merge remote-tracking branch 'dev/1-58-stable' into 1-58-stable1-58-stable
| -rw-r--r-- | CHANGELOG.md | 6 | ||||
| -rw-r--r-- | VERSION | 2 | ||||
| -rw-r--r-- | internal/artifact/artifact.go | 6 | ||||
| -rw-r--r-- | internal/artifact/artifact_test.go | 25 |
4 files changed, 33 insertions, 6 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f044e9d..9e3c9bfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.58.1 (2022-07-12) + +### Security (1 change) + +- [Include remote address through X-Forwarded-For in job artifact request](gitlab-org/security/gitlab-pages@42f01f5365b65058faa5d3e53681c06034dfe3dd) ([merge request](gitlab-org/security/gitlab-pages!40)) + ## 1.58.0 (2022-05-17) ### Changed (3 changes) @@ -1 +1 @@ -1.58.0 +1.58.1 diff --git a/internal/artifact/artifact.go b/internal/artifact/artifact.go index dab1fb91..e33a9ab9 100644 --- a/internal/artifact/artifact.go +++ b/internal/artifact/artifact.go @@ -17,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/logging" + "gitlab.com/gitlab-org/gitlab-pages/internal/request" ) const ( @@ -88,6 +89,11 @@ func (a *Artifact) makeRequest(w http.ResponseWriter, r *http.Request, reqURL *u if token != "" { req.Header.Add("Authorization", "Bearer "+token) } + + // The GitLab API expects this value for Group IP restriction to work properly + // on requests coming through Pages. + req.Header.Set("X-Forwarded-For", request.GetRemoteAddrWithoutPort(r)) + resp, err := a.client.Do(req) if err != nil { if errors.Is(err, context.Canceled) { diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go index b8c2771b..472c0d7b 100644 --- a/internal/artifact/artifact_test.go +++ b/internal/artifact/artifact_test.go @@ -16,8 +16,6 @@ import ( func TestTryMakeRequest(t *testing.T) { content := "<!DOCTYPE html><html><head><title>Title of the document</title></head><body></body></html>" contentType := "text/html; charset=utf-8" - testServer := makeArtifactServerStub(t, content, contentType) - defer testServer.Close() cases := []struct { Path string @@ -28,6 +26,8 @@ func TestTryMakeRequest(t *testing.T) { CacheControl string ContentType string Description string + RemoteAddr string + ForwardedIP string }{ { "/200.html", @@ -38,6 +38,8 @@ func TestTryMakeRequest(t *testing.T) { "max-age=3600", "text/html; charset=utf-8", "basic successful request", + "1.2.3.4:8000", + "1.2.3.4", }, { "/200.html", @@ -48,6 +50,8 @@ func TestTryMakeRequest(t *testing.T) { "", "text/html; charset=utf-8", "basic successful request", + "1.2.3.4", + "1.2.3.4", }, { "/max-caching.html", @@ -58,6 +62,8 @@ func TestTryMakeRequest(t *testing.T) { "max-age=3600", "text/html; charset=utf-8", "max caching request", + "1.2.3.4", + "1.2.3.4", }, { "/non-caching.html", @@ -68,17 +74,24 @@ func TestTryMakeRequest(t *testing.T) { "", "text/html; charset=utf-8", "no caching request", + "1.2.3.4", + "1.2.3.4", }, } for _, c := range cases { t.Run(c.Description, func(t *testing.T) { - result := httptest.NewRecorder() + testServer := makeArtifactServerStub(t, content, contentType, c.ForwardedIP) + defer testServer.Close() + reqURL, err := url.Parse("/-/subgroup/project/-/jobs/1/artifacts" + c.Path) require.NoError(t, err) r := &http.Request{URL: reqURL} + r.RemoteAddr = c.RemoteAddr art := artifact.New(testServer.URL, 1, "gitlab-example.io") + result := httptest.NewRecorder() + require.True(t, art.TryMakeRequest("group.gitlab-example.io", result, r, c.Token, func(resp *http.Response) bool { return false })) require.Equal(t, c.Status, result.Code) require.Equal(t, c.ContentType, result.Header().Get("Content-Type")) @@ -90,8 +103,10 @@ func TestTryMakeRequest(t *testing.T) { } // provide stub for testing different artifact responses -func makeArtifactServerStub(t *testing.T, content string, contentType string) *httptest.Server { +func makeArtifactServerStub(t *testing.T, content string, contentType string, expectedForwardedIP string) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, expectedForwardedIP, r.Header.Get("X-Forwarded-For")) + w.Header().Set("Content-Type", contentType) switch r.URL.RawPath { case "/projects/group%2Fsubgroup%2Fproject/jobs/1/artifacts/200.html": @@ -280,7 +295,7 @@ func TestBuildURL(t *testing.T) { func TestContextCanceled(t *testing.T) { content := "<!DOCTYPE html><html><head><title>Title of the document</title></head><body></body></html>" contentType := "text/html; charset=utf-8" - testServer := makeArtifactServerStub(t, content, contentType) + testServer := makeArtifactServerStub(t, content, contentType, "") t.Cleanup(testServer.Close) result := httptest.NewRecorder() |