Skip to content

feat(events): add information on X-Forwarded-For header #907

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

Merged
merged 2 commits into from
May 6, 2025
Merged

Conversation

bt90
Copy link
Contributor

@bt90 bt90 commented Apr 11, 2025

Describe the event structure if a reverse proxy is involved.

calmh added a commit to syncthing/syncthing that referenced this pull request Apr 23, 2025
### Purpose

Fix #9336

The `emitLoginAttempt` function now checks for the presence of an
`X-Forwarded-For` header. The IP from this header is only used if the
connecting host is either on loopback or on the same LAN.

In the case of a host pretending to be a proxy, we'd still have both IPs
in the logs, which should make this much less critical from a security
standpoint.

### Testing

1. directly via localhost
2. via proxy an localhost

#### Logs

```
[3JPXJ] 2025/04/11 15:00:40 INFO: Wrong credentials supplied during API authorization from 127.0.0.1
[3JPXJ] 2025/04/11 15:03:04 INFO: Wrong credentials supplied during API authorization from 192.168.178.5 proxied by 127.0.0.1
```

#### Event API

```
  {
    "id": 23,
    "globalID": 23,
    "time": "2025-04-11T15:00:40.578577402+02:00",
    "type": "LoginAttempt",
    "data": {
      "remoteAddress": "127.0.0.1",
      "success": false,
      "username": "sdfsd"
    }
  },
  {
    "id": 24,
    "globalID": 24,
    "time": "2025-04-11T15:03:04.423403976+02:00",
    "type": "LoginAttempt",
    "data": {
      "proxy": "127.0.0.1",
      "remoteAddress": "192.168.178.5",
      "success": false,
      "username": "sdfsd"
    }
  }
```

### Documentation

syncthing/docs#907

---------

Co-authored-by: Jakob Borg <[email protected]>
@bt90
Copy link
Contributor Author

bt90 commented May 6, 2025

@calmh can we merge here as the changes are already live with v1.29.6?

@calmh
Copy link
Member

calmh commented May 6, 2025

Of course. Y'all don't need to gate everything on me.

@bt90
Copy link
Contributor Author

bt90 commented May 6, 2025

I'd merge if I could 😅 #904 and this PR are both blocked by policy-bot despite having an approval as far as I can see.

@tomasz1986 tomasz1986 changed the title x-forwarded-for feat(events): add information on X-Forwarded-For header May 6, 2025
@bt90 bt90 enabled auto-merge (squash) May 6, 2025 09:51
@bt90 bt90 merged commit 34de465 into main May 6, 2025
3 checks passed
@bt90 bt90 deleted the bt90-patch-2 branch May 6, 2025 09:51
@marbens-arch
Copy link
Member

#904 and this PR are both blocked by policy-bot despite having an approval as far as I can see.

#904 was approved by me before I had write access. policy-bot may not have retroactively counted it.

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 this pull request may close these issues.

4 participants