From ca1d7e438c569945e795d6182a87a143712e5da3 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Fri, 9 Jun 2023 09:02:34 -0400 Subject: [PATCH 1/2] Shielding subject folder (#37690) --- .github/workflows/test.yml | 1 + middleware/index.js | 8 +- .../middleware/handle-invalid-paths.js | 0 .../handle-invalid-query-strings.js | 2 +- src/shielding/middleware/index.js | 13 ++++ .../shielding/middleware}/rate-limit.js | 2 +- .../shielding/tests}/invalid-querystrings.js | 5 +- src/shielding/tests/shielding.js | 76 +++++++++++++++++++ 8 files changed, 96 insertions(+), 11 deletions(-) rename src/{observability => shielding}/middleware/handle-invalid-paths.js (100%) rename src/{observability => shielding}/middleware/handle-invalid-query-strings.js (98%) create mode 100644 src/shielding/middleware/index.js rename {middleware => src/shielding/middleware}/rate-limit.js (98%) rename {tests/rendering-fixtures => src/shielding/tests}/invalid-querystrings.js (91%) create mode 100644 src/shielding/tests/shielding.js diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7cd009631b..469ff7c72f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -57,6 +57,7 @@ jobs: { name: 'rest', path: 'src/rest/tests', }, { name: 'routing', path: 'tests/routing', }, { name: 'search', path: 'src/search/tests', }, + { name: 'shielding', path: 'src/shielding/tests', }, context.payload.repository.full_name === 'github/docs-internal' && { name: 'translations', path: 'tests/translations', }, { name: 'unit', path: 'tests/unit', }, diff --git a/middleware/index.js b/middleware/index.js index 31451ce92d..02167f64c7 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -16,7 +16,6 @@ import { setLanguageFastlySurrogateKey, } from './set-fastly-surrogate-key.js' import handleErrors from '#src/observability/middleware/handle-errors.js' -import handleInvalidPaths from '#src/observability/middleware/handle-invalid-paths.js' import handleNextDataPath from './handle-next-data-path.js' import detectLanguage from './detect-language.js' import reloadTree from './reload-tree.js' @@ -66,8 +65,7 @@ import fastlyBehavior from './fastly-behavior.js' import mockVaPortal from './mock-va-portal.js' import dynamicAssets from './dynamic-assets.js' import contextualizeSearch from '#src/search/middleware/contextualize.js' -import rateLimit from './rate-limit.js' -import handleInvalidQuerystrings from '#src/observability/middleware/handle-invalid-query-strings.js' +import shielding from '#src/shielding/middleware/index.js' const { DEPLOYMENT_ENV, NODE_ENV } = process.env const isTest = NODE_ENV === 'test' || process.env.GITHUB_ACTIONS === 'true' @@ -201,9 +199,7 @@ export default function (app) { } // *** Early exits *** - app.use(rateLimit) - app.use(instrument(handleInvalidQuerystrings, './handle-invalid-querystrings')) - app.use(instrument(handleInvalidPaths, './handle-invalid-paths')) + app.use(shielding) app.use(instrument(handleNextDataPath, './handle-next-data-path')) // *** Security *** diff --git a/src/observability/middleware/handle-invalid-paths.js b/src/shielding/middleware/handle-invalid-paths.js similarity index 100% rename from src/observability/middleware/handle-invalid-paths.js rename to src/shielding/middleware/handle-invalid-paths.js diff --git a/src/observability/middleware/handle-invalid-query-strings.js b/src/shielding/middleware/handle-invalid-query-strings.js similarity index 98% rename from src/observability/middleware/handle-invalid-query-strings.js rename to src/shielding/middleware/handle-invalid-query-strings.js index 1ca0b8faf1..43a6e22648 100644 --- a/src/observability/middleware/handle-invalid-query-strings.js +++ b/src/shielding/middleware/handle-invalid-query-strings.js @@ -1,4 +1,4 @@ -import statsd from '../lib/statsd.js' +import statsd from '#src/observability/lib/statsd.js' import { noCacheControl, defaultCacheControl } from '../../../middleware/cache-control.js' const STATSD_KEY = 'middleware.handle_invalid_querystrings' diff --git a/src/shielding/middleware/index.js b/src/shielding/middleware/index.js new file mode 100644 index 0000000000..7f4b8f10ff --- /dev/null +++ b/src/shielding/middleware/index.js @@ -0,0 +1,13 @@ +import express from 'express' + +import handleInvalidQuerystrings from './handle-invalid-query-strings.js' +import handleInvalidPaths from './handle-invalid-paths.js' +import rateLimit from './rate-limit.js' + +const router = express.Router() + +router.use(rateLimit) +router.use(handleInvalidQuerystrings) +router.use(handleInvalidPaths) + +export default router diff --git a/middleware/rate-limit.js b/src/shielding/middleware/rate-limit.js similarity index 98% rename from middleware/rate-limit.js rename to src/shielding/middleware/rate-limit.js index 88bf7e60d9..7ca847e305 100644 --- a/middleware/rate-limit.js +++ b/src/shielding/middleware/rate-limit.js @@ -1,7 +1,7 @@ import rateLimit from 'express-rate-limit' import statsd from '#src/observability/lib/statsd.js' -import { noCacheControl } from './cache-control.js' +import { noCacheControl } from '../../../middleware/cache-control.js' const EXPIRES_IN_AS_SECONDS = 60 diff --git a/tests/rendering-fixtures/invalid-querystrings.js b/src/shielding/tests/invalid-querystrings.js similarity index 91% rename from tests/rendering-fixtures/invalid-querystrings.js rename to src/shielding/tests/invalid-querystrings.js index c40fd728f0..e26ab6c5af 100644 --- a/tests/rendering-fixtures/invalid-querystrings.js +++ b/src/shielding/tests/invalid-querystrings.js @@ -1,10 +1,9 @@ -import { describe } from '@jest/globals' +import { get } from '../../../tests/helpers/e2etest.js' -import { get } from '../helpers/e2etest.js' import { MAX_UNFAMILIAR_KEYS_BAD_REQUEST, MAX_UNFAMILIAR_KEYS_REDIRECT, -} from '#src/observability/middleware/handle-invalid-query-strings.js' +} from '#src/shielding/middleware/handle-invalid-query-strings.js' const alpha = Array.from(Array(26)).map((e, i) => i + 65) const alphabet = alpha.map((x) => String.fromCharCode(x)) diff --git a/src/shielding/tests/shielding.js b/src/shielding/tests/shielding.js new file mode 100644 index 0000000000..307f98f47d --- /dev/null +++ b/src/shielding/tests/shielding.js @@ -0,0 +1,76 @@ +import { get } from '../../../tests/helpers/e2etest.js' + +describe('honeypotting', () => { + test('any GET with survey-vote and survey-token query strings is 400', async () => { + const res = await get('/en?survey-vote=1&survey-token=2') + expect(res.statusCode).toBe(400) + expect(res.body).toMatch(/Honeypotted/) + expect(res.headers['cache-control']).toMatch('private') + }) +}) + +describe('junk paths', () => { + test('junk full pathname', async () => { + const res = await get('/xmlrpc.php') + expect(res.statusCode).toBe(404) + expect(res.headers['content-type']).toMatch('text/plain') + expect(res.headers['cache-control']).toMatch('public') + }) + + test('junk base name', async () => { + const res = await get('/en/get-started/.env.local') + expect(res.statusCode).toBe(404) + expect(res.headers['content-type']).toMatch('text/plain') + expect(res.headers['cache-control']).toMatch('public') + }) + + test.each(['/_nextanything', '/_next/data', '/_next/data/'])( + 'invalid requests for _next prefix %s', + async (path) => { + const res = await get(path) + expect(res.statusCode).toBe(404) + expect(res.headers['content-type']).toMatch('text/plain') + expect(res.headers['cache-control']).toMatch('public') + } + ) + + test('any URL that ends with /index.md redirects', async () => { + const res = await get('/en/get-started/index.md') + expect(res.statusCode).toBe(302) + expect(res.headers.location).toBe('/en/get-started') + }) +}) + +describe('rate limiting', () => { + // We can't actually trigger a full rate limit because + // then all other tests will all fail. And we can't rely on this + // test always being run last. + + test('only happens if you have junk query strings', async () => { + const res = await get('/robots.txt?foo=bar') + expect(res.statusCode).toBe(200) + const limit = parseInt(res.headers['ratelimit-limit']) + const remaining = parseInt(res.headers['ratelimit-remaining']) + expect(limit).toBeGreaterThan(0) + expect(remaining).toBeLessThan(limit) + + // A second request + { + const res = await get('/robots.txt?foo=buzz') + expect(res.statusCode).toBe(200) + const newLimit = parseInt(res.headers['ratelimit-limit']) + const newRemaining = parseInt(res.headers['ratelimit-remaining']) + expect(newLimit).toBe(limit) + // Can't rely on `newRemaining == remaining - 1` because of + // concurrency of jest-running. + expect(newRemaining).toBeLessThan(remaining) + } + }) + + test('nothing happens if no unrecognized query string', async () => { + const res = await get('/robots.txt') + expect(res.statusCode).toBe(200) + expect(res.headers['ratelimit-limit']).toBeUndefined() + expect(res.headers['ratelimit-remaining']).toBeUndefined() + }) +}) From de44b2843e8ec4f13653864bc7d659815bee7404 Mon Sep 17 00:00:00 2001 From: Sophie <29382425+sophietheking@users.noreply.github.com> Date: Fri, 9 Jun 2023 15:08:32 +0200 Subject: [PATCH 2/2] [Improvement]: Update document to include contributions that cannot be linked to new account (#37700) --- ...hy-are-my-contributions-not-showing-up-on-my-profile.md | 1 + .../merging-multiple-personal-accounts.md | 7 ++++--- .../pull_requests/pull_request_merges_and_contributions.md | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/content/account-and-profile/setting-up-and-managing-your-github-profile/managing-contribution-settings-on-your-profile/why-are-my-contributions-not-showing-up-on-my-profile.md b/content/account-and-profile/setting-up-and-managing-your-github-profile/managing-contribution-settings-on-your-profile/why-are-my-contributions-not-showing-up-on-my-profile.md index da01803d9e..1ed832fcef 100644 --- a/content/account-and-profile/setting-up-and-managing-your-github-profile/managing-contribution-settings-on-your-profile/why-are-my-contributions-not-showing-up-on-my-profile.md +++ b/content/account-and-profile/setting-up-and-managing-your-github-profile/managing-contribution-settings-on-your-profile/why-are-my-contributions-not-showing-up-on-my-profile.md @@ -29,6 +29,7 @@ If you are part of an organization that uses SAML single sign-on (SSO), you won Issues, pull requests, and discussions will appear on your contribution graph if they were opened in a standalone repository, not a fork. ### Commits + Commits will appear on your contributions graph if they meet **all** of the following conditions: - The email address used for the commits is associated with your account on {% data variables.location.product_location %}. - The commits were made in a standalone repository, not a fork. diff --git a/content/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-personal-account/merging-multiple-personal-accounts.md b/content/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-personal-account/merging-multiple-personal-accounts.md index 32e914a892..e5e428c15d 100644 --- a/content/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-personal-account/merging-multiple-personal-accounts.md +++ b/content/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-personal-account/merging-multiple-personal-accounts.md @@ -24,7 +24,7 @@ shortTitle: Merge multiple accounts {% else %} -**Tip:** We recommend using only one personal account to manage both personal and professional repositories. +**Tip:** We recommend using only one personal account to manage both personal and professional repositories. {% endif %} @@ -32,9 +32,10 @@ shortTitle: Merge multiple accounts {% warning %} -**Warning:** +**Warning:** - Organization and repository access permissions aren't transferable between accounts. If the account you want to delete has an existing access permission, an organization owner or repository administrator will need to invite the account that you want to keep. -- Any commits authored with a GitHub-provided `noreply` email address cannot be transferred from one account to another. If the account you want to delete used the **Keep my email address private** option, it won't be possible to transfer the commits authored by the account you are deleting to the account you want to keep. +- Any commits authored with a {% data variables.product.company_short %}-provided `noreply` email address cannot be transferred from one account to another. If the account you want to delete used the **Keep my email address private** option, it won't be possible to transfer the commits authored by the account you are deleting to the account you want to keep. +- Issues, pull requests, and discussions will not be attributed to the new account. {% endwarning %} diff --git a/data/reusables/pull_requests/pull_request_merges_and_contributions.md b/data/reusables/pull_requests/pull_request_merges_and_contributions.md index e157092539..81bef02ac8 100644 --- a/data/reusables/pull_requests/pull_request_merges_and_contributions.md +++ b/data/reusables/pull_requests/pull_request_merges_and_contributions.md @@ -2,6 +2,7 @@ **Notes:**{% ifversion ghes or ghae %} - To appear on your profile contributions graph, co-authored commits must meet the same criteria as commits with one author.{% endif %} -- When rebasing commits, the original authors of the commit and the person who rebased the commits, whether on the command line or on {% data variables.location.product_location %}, receive contribution credit. +- When rebasing commits, the original authors of the commit and the person who rebased the commits, whether on the command line or on {% data variables.location.product_location %}, receive contribution credit.{% ifversion ghec or fpt %} +- If you merged multiple personal accounts, issues, pull requests, and discussions will not be attributed to the new account and will not appear on your contribution graph.{% endif %} {% endnote %}